Je pense que le problème est plus haut et que ce changement ici peut être une erreur (car il y a des sorties de boucle "prématurés" avec les break dans ce while plus haut):

      while (  (ref = ref->GetPrev())
            && ref->IsNotNullChunk())
      {

Je suppose que ref ne peut pas devenir null et donc

      while ( (ref = ref->GetPrev())->IsNotNullChunk())
      {

est la solution qui permets de rester au plus proche de ce qui est déjà fait avec moins de risque d'effets de bord.

The two snippers of code in the comment above do exactly the same thing. Why would the second be better than the first? It reduce code readability.

If we get at this point of the code, ref is definitely not a null chuck, so the propose code from Guy is correct.

The original code looks like this:

      while (  (ref = ref->GetPrev())
            && ref->IsNotNullChunk())
      {
          if(some_other_condition) { break; }
      }
      if (  ref->IsNullChunk()
         && !Chunk::GetHead()->IsComment()
         && Chunk::GetHead()->GetParentType() == type)
      {
      // [...]
      }

For the compiler, the static analysis tool and any first inspecxtion of the code, this is a test if (ref = ref->GetPrev()) is not null, and if true, a test that ref->IsNotNullChunk() is not "falsy".

So it looks like the while loop can end when ref became NULL and therefore the test ref->IsNullChunk() in the test after the loop would reference a NULL pointer.
So cppcheck indicates that this looks like an issue.

Readabomotu os a point of view, but the next alternative is only one test to the compiler and the static analysis tool - and also suggests to the reager that ref can not be null (which can be reinforced with a comment).

while ((ref = ref->GetPrev())->IsNotNullChunk())

The following is an alternative: still two statements but only one test.

while (ref = ref->GetPrev(), ref->IsNotNullChunk())
{

My other observation is that removing the ref->IsNullChunk() can have other side effects as the while loop can exit when that conditioni is false, so removing the condition from the test after the loop requires deeper analysis to ensure that the behavior did not change.

@micheleCTDEAdmin indicats:

If we get at this point of the code, ref is definitely not a null chuck, so the propose code from Guy is correct.

In the original while test I see ref->IsNotNullChunk(). So I conclude that the loop can exit with:

• ref->IsNullChunk() being true (so it would be a null chunk) when exited because of the loop condition,
• but ref->IsNullChunk() being false if the loop exits because of a break; inside the loop.

Of course the test cases pass, so theyt do not proof that ref->IsNullChunk() can be false or have a negative side-effect if it is false.

However, I leave the in-depth analysis up to you as you're more familiar with the code.

ref is a Chuck. A null is a valid Chunk and the way the "null chuck" logic has been designed, it ensures that ref will always point to a Chunk object, which could be the NullChunk. A NullChuck is not a nullptr, by the way.
I looked at the code earlier today and indeed at the point ref will point to a valid chuck, because otherwise the while loop would have either ended or skipped the iteration

• If the while loop ends because of a break, then 'ref->IsNullChunk()' is false and therefore with this condition removed from the test on line 1862, the statements in that test block are now executed (before they were not).
• Apparently according to your analysis, the loop can never end because of 'ref->IsNotNullChunk() returning false because after the loop the Chunk is always a valid Chuck, so then this test is futile and this basically is the same as while(1) {ref=ref->GetPrev(); /* rest of block */;
• The NullChuck is a Null Object pattern. So the test on "ref = ref->GetPrev()" not being null is futile.

I have checked again, the first time I must have been confused by the indentation and I made a wrong comment. If we get at line 1862, the chuck may or may not be a NullChunk, depending on how the while loop terminated. So we need to keep the check for NullChunk in the "if":

if (  ref->IsNullChunk()
   && !Chunk::GetHead()->IsComment()

Apparently according to your analysis, the loop can never end because of 'ref->IsNotNullChunk() returning false because after the loop the Chunk is always a valid Chuck

This is incorrect. The NullChunk is a valid chunk object, but it has a special meaning. so the check for a chuck to be or not to be the NullChunk is very meaningful.

Apparently according to your analysis, the loop can never end because of 'ref->IsNotNullChunk() returning false because after the loop the Chunk is always a valid Chuck

This is incorrect. The NullChunk is a valid chunk object, but it has a special meaning. so the check for a chuck to be or not to be the NullChunk is very meaningful.

So we agree on this ;-) . My observation was that based on the fact that the test ref->IsNullChunk() was removed (implicitly assuming that is is true) that the new code assumed that ref->IsNotNullChunk() is always false (opposite) leading to the conclusion that keeping ref->IsNullChunk() is the right thing to do.

yes, we agree on this point. Sorry for the initial wrong comment.

Something is very wrong here, even in the original code. At line 1405 we check if tmp2 is not the null chunk. If it is not, we execute the if-body at lines 1406-1414. If the if failed, we enter the else-body at lines 1416-1422. Setting tmp2 to CT_FUNC_TYPE is wrong, because tmp2 is a null chuck, it can't be a function.
The original code is wrong as well, but at least the if at original line 1420 would have prevented the additional problem, because at that line tmp2 is indeed the null chuck.

Actually we can completely remove the original 4 lines of code at 1420-1423, because that "if" was always false.

This needs to be changed before we can merge it, or the PR will introduce a serious bug because it could set the type of the NullChunk to CT_FUNC_TYPE, which is obviously wrong.

The two snippers of code in the comment above do exactly the same thing. Why would the second be better than the first? It reduce code readability.

If we get at this point of the code, ref is definitely not a null chuck, so the propose code from Guy is correct.