Fix #671: Missing newline at end of file should produce a warning

[glankk]

No description provided.

[firewave]

All these stray -1 and \n are really awful and make this feel very wrong.

[danmar]

All these stray -1 and \n are really awful and make this feel very wrong.

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The \n makes the testcode technically valid. Are you suggestion to remove those and ensure that warnings are written properly?

[firewave]

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

Me neither. Passing less means that the newline would be missing and the other changes add it. That is kind of a flip-flop.

The \n makes the testcode technically valid. Are you suggestion to remove those and ensure that warnings are written properly?

If fiddling with the trailing newline is necessary that seems like a possible source of issues for users of the library.

[glankk]

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The char arrays that are initialized with a string constant include a null terminator, which makes these test cases fail because they end with '\0' instead of '\n'.

All these stray -1 and \n are really awful and make this feel very wrong.

I agree that it looks really ugly but it's not wrong.

[glankk]

I also agree that it's a bit awkward to implement this in simplecpp, since it doesn't really distinguish between C and C++.

[firewave]

I am not sure why the -1 are needed. Not saying it's wrong, I just don't understand it.

The char arrays that are initialized with a string constant include a null terminator, which makes these test cases fail because they end with '\0' instead of '\n'.

All these stray -1 and \n are really awful and make this feel very wrong.

I agree that it looks really ugly but it's not wrong.

Maybe we should something more explicit to highlight what we are doing and it doesn't look like we randomly added those. We should probably land #666 first which allows passing the actual sizes instead of just relying onto strlen().

[danmar]

If fiddling with the trailing newline is necessary that seems like a possible source of issues for users of the library.

Of course we should write test code properly. If the newline is not included in the test code then it is not included.
Let's write test code exactly as we want it to be.

All these stray -1 and \n are really awful and make this feel very wrong.
I agree that it looks really ugly but it's not wrong.

It's not "ugly" to write test code properly.

I understand now why you write sizeof(code)-1 and that is the proper way to calculate the size.

I.e. if we have const char code[] = "abcde"; then sizeof(code) will return 6. If we pass 6 as size argument to the preprocess function then we tell the preprocess function that it should read the null character after the 'e' when preprocessing. If we don't intended that then we should not pass 6.

[danmar]

I also agree that it's a bit awkward to implement this in simplecpp, since it doesn't really distinguish between C and C++.

I want that we are able to find this UB. But we do need to distinguish C/C++ somewhere. I can see these options:

• add a flag for TokenList so that it knows if it is C code. Maybe as a parameter to the constructors.
• instead of emitting NO_EOF_NEWLINE, we could have a Location list in TokenList called missingNewline. Tools can inspect that list after preprocessing to determine if they should write warnings.
• current solution: simplecpp will emit the NO_EOF_NEWLINE portability warning, and the tool must determine what it should do about that. I.e. Cppcheck will just it for C++ files.

current solution seems OK to me.

[danmar]

we could adjust main.cpp so that this output is only written if the filename extension is ".c".

[glankk]

I think this is a bit of a clumsy way to solve this issue, I know there exists production code where c++ files have a .c extension (gdb is one example of this). We could have command-line flag for choosing the standard to use for simplecpp, but I think it might be better to just suppress this in the simplecpp executable and let cppcheck choose how to handle this when thrown from the simplecpp library. As far as I know neither the gcc or clang preprocessor have a warning for this, so there's precedence.

[glankk]

Sorry, I just realized we do have a -std flag for simplecpp. We should probably use that to decide what to do with this warning, and use the filename as a backup for autodetecting the standard to use.

[danmar]

main.cpp is an "example implementation" more or less. It doesn't have to be perfect. If the -std option has been specified then please rely on that primarily.

Cppcheck has both --std and --language. It has better knowledge about the language than simplecpp.
The --language option is not communicated to simplecpp.

[glankk]

I suggest something like this:

case simplecpp::Output::PORTABILITY_NO_EOF_NEWLINE:
  if (simplecpp::getCStd(dui.std) == simplecpp::CUnknown) {
      // Only UB for c code, suppress for c++ code
      // If no standard is specified then prefer to have a false negative
      continue;
  }
  std::cerr << "portability: ";
  break;

cppcheck will have it's own way of handling this obviously, but I think this is okay for the simplecpp executable.

[danmar]

I know there exists production code where c++ files have a .c extension

yes sure. And in these cases you would use the cppcheck option --language=c++ .

If you just execute cppcheck --std=c++11 path then you say that c++ files in the path should be analyzed using "c++11". and c files in the path are analyzed with default C standard.