locked
Solving “warning: not all control paths return a value” mean?" RRS feed

  • Question

  • Hello Programmers,

    I am facing a warning C4715: CreateAndTrain::cSomeClassContainer::SomeFunction' : not all control paths return a value. 

    I realized that  there should be valid return type if, "if" condition fails.

    But i could not understand what return type i should use for ref class ?

    Below is class - 

    SomeClass^ cSomeClassContainer::someFunction(System::Guid cID)
    {
    	for each(CNameClass^ cName in this->NameList) {
    		
    		if(cName->Name->ID==cID)
    		{
    			return cName->Name;
    		}
    		else
    		{
    			Console::WriteLine("Unidentified ID");
    			//return;      <--- Here ??
    		}
    	}
    }

    Where, 

    public ref class CNameClass
    {
    public:
    	property SomeCLass^ Name;													// Single Label
    };

    Thanks in Advance.......


    • Edited by Dipak_S Monday, September 14, 2015 9:44 AM
    Monday, September 14, 2015 9:43 AM

Answers

  • The same as cName->Name. But I guess it would be better to use a single point of return and write it as (aircode):

    SomeClass^ cSomeClassContainer::someFunction(System::Guid cID)
    {
    	SomeClass^ *result = NULL;
    	for each(CNameClass^ cName in this->NameList)
    	{		
    		if(cName->Name->ID==cID)
    		{
    			*p = cName->Name;
    			break;
    		}		
    	}
    	
    	return result;
    }

    Otherwise your logic would have a flaw.

    • Proposed as answer by rp_suman Wednesday, September 16, 2015 6:50 AM
    • Marked as answer by May Wang - MSFT Monday, September 28, 2015 7:54 AM
    Monday, September 14, 2015 9:58 AM

All replies

  • The same as cName->Name. But I guess it would be better to use a single point of return and write it as (aircode):

    SomeClass^ cSomeClassContainer::someFunction(System::Guid cID)
    {
    	SomeClass^ *result = NULL;
    	for each(CNameClass^ cName in this->NameList)
    	{		
    		if(cName->Name->ID==cID)
    		{
    			*p = cName->Name;
    			break;
    		}		
    	}
    	
    	return result;
    }

    Otherwise your logic would have a flaw.

    • Proposed as answer by rp_suman Wednesday, September 16, 2015 6:50 AM
    • Marked as answer by May Wang - MSFT Monday, September 28, 2015 7:54 AM
    Monday, September 14, 2015 9:58 AM
  • Hello Programmers,

    I am facing a warning C4715: CreateAndTrain::cSomeClassContainer::SomeFunction' : not all control paths return a value. 

    I realized that  there should be valid return type if, "if" condition fails.

    But i could not understand what return type i should use for ref class ?

    Below is class - 

    SomeClass^ cSomeClassContainer::someFunction(System::Guid cID)
    {
    	for each(CNameClass^ cName in this->NameList) {
    		
    		if(cName->Name->ID==cID)
    		{
    			return cName->Name;
    		}
    		else
    		{
    			Console::WriteLine("Unidentified ID");
    			//return;      <--- Here ??
    		}
    	}
    }

    Where, 

    public ref class CNameClass
    {
    public:
    	property SomeCLass^ Name;													// Single Label
    };

    Shouldn't you just return nullptr?

    David Wilkinson | Visual C++ MVP

    Monday, September 14, 2015 5:36 PM
  • This mean inside your code you have conditional statement and one of those statement is missing return type. And you function is expecting a return type as per your function signature.

    Thanks


    Rupesh Shukla

    Tuesday, September 15, 2015 1:26 PM
  • You are not returning anything from the function if no match was found inside for loop. If you try to access the return value at run time without checking its validity, the program may crash or behavior is undefined.

    As Stefan Hoffmann advised, It is always good to have a variable of return type initialized and use that to return at one place which will make the code readable and assembly code better.

    If you practice, first create a flowchart for your problem and then create code using that, you can avoid these programming mistakes in most cases.


    Thanks & Regards

    Wednesday, September 16, 2015 6:49 AM
  • eYou are not returning anything from the function if no match was found inside for loop. If you try to access the return value at run time without checking its validity, the program may crash or behavior is undefined.

    As Stefan Hoffmann advised, It is always good to have a variable of return type initialized and use that to return at one place which will make the code readable and assembly code better.

    I don't believe that "single point of return" is universally accepted as desirable style.

    More importantly, the logic of OP's code is incorrect -- the negative return is in the wrong place. This is corrected but not mentioned in Stefan Hoffman's post (which also contains an error: variable 'p' should be 'result').

    I would write it as

    SomeClass^ CSomeClassContainer::SomeFunction(System::Guid cID)
    {
        for each(CNameClass^ cName in this->NameList)
        {        
            if(cName->Name->ID == cID)
            {
                return cName->Name;
            }        
        }
        return nullptr;
    }
    

    Personally I prefer this immediate return when a match is found.


    David Wilkinson | Visual C++ MVP

    Wednesday, September 16, 2015 8:07 AM
  • p was a typo, I've just typed it right down. No VS, no error checks. Thanks for pointing it out.

    Single point of return, just my thoughts:

    Single point of return is from the old ages where structural programming was the thing.

    But it is a good method to write a method's body. Multiple exit points are often an indicator for a fat method, which should be refactored. Refactoring a method with multiple exit points is harder than with one. So using SPOR allows us to start in a top-down fashion to create functionality first. I can easily fill in further code blocks without breaking the flow. But allows also easier modification/refactoring later.

    When we have a clean, "atomic" method (Rules of Functions), then using more than one exit point is okay. As it is clear to see what is happening.

    Wednesday, September 16, 2015 9:14 AM