In D54757#1305306, @Eugene.Zelenko wrote:

In D54757#1305114, @whisperity wrote:

Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM?

GCC 7 introduced -Wduplicated-branches, so will be good idea to run this check on associated regression(s).

Bar the previous comments, I really like this. The test suite is massive and well-constructed. Do we know of any real-world findings, maybe even from LLVM?

Let's register the ID...

In D53974#1294385, @ztamas wrote:

I also tested on LLVm code.
The output is here:
https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4

I found 362 warnings.

Around 95% of these warnings are similar to the next example:

/home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop variable has narrower type 'unsigned int' than iteration's upper bound 'size_t' (aka 'unsigned long') [bugprone-too-small-loop-variable]
  for (unsigned I = 0; I < Style.size(); ++I) {

Where the loop variable has an unsigned int type while in the loop condition it is compared with a container size which has size_t type. The actual size method can be std::string::length() or array_lengthof() too.

[snip snip]

I can't see similar false positives what LibreOffice code produces.

Yes, down the line I realised the function is not needed. (It emits a diagnostic because the diagnostic comes from comparing the AST file's config blocks to the current (at the time of import) compilation.)

Test was added.

In D53334#1288057, @dblaikie wrote:

In D53334#1273877, @whisperity wrote:

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm -o file.pcm

However, using %clang_cc1 I can't get it to accept --precompile as a valid argument, and using %clang_cpp I get an "unused argument" warning for --precompile and the output file is just a preprocessed output (like -E), which will, of course, cause errors, but not the errors I am wanting to test about.

Hey, sorry for the delay - you can use "clang -# <other arguments>" (or "clang++ -# <other arguments>" to get clang to print out the underlying -cc1 command line that is used.

If you're changing the driver then we'd write a driver test (that tests that, given "clang -foo -###" it produces some "clang -cc1 -bar" command line to run the frontend.

But since you're changing the driver, it's not interesting to (re) test how --precompile is lowered from the driver to cc1. Instead we test the frontend (cc1) directly.

In D45050#1280831, @dyung wrote:

I have attached a bzipped preprocessed file that I can confirm will show the failure if built with clang++ version 3.7.1, specifically the version that shipped with ubuntu 14.04.5 LTS. Here is the full version information:

Ubuntu clang version 3.7.1-svn253742-1~exp1 (branches/release_37) (based on LLVM 3.7.1)

Target: x86_64-pc-linux-gnu
Thread model: posix

If you build it with “clang++ -std=c++11 NotNullTerminatedResultCheck.preproc.cpp” you should see the failure.

Updating the diff just in case so that I don't lose the test code.

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

Looks good.

With regards to D53352: you can change the diff related to a patch whilst keeping discuccion and metadata of the diff.

Remove the custom file paths and URLs but other than that this is pretty trivial.

@aaron.ballman Neither I nor @Charusso has commit rights, could you please commit this in our stead?

Soft ping.

Have been looked at this far and wide too many times now. I say we roll this out and fix and improve later on, lest this only going into Clang 72. Results look promising, let's hope users start reporting good findings too.

Looks good. This change seems very beneficial, I have a little downstream project I'll have to commit an update for when this releases but it is a trivial change on the users' perspective.

Are you sure the diff is okay? I see only a move on VirtualFileSystem.cpp but I really think there were namespace clang { lines in it too.

Your code looks good, just minor comments going on inline.
Trying to think of more cases to test for, in case someone generously misuses FLMs, as seen here in an example if you scroll down a bit: https://gcc.gnu.org/onlinedocs/cpp/Macro-Arguments.html#Macro-Arguments
Maybe one or two tests cases for this should be added, but I believe all corners are covered other than this.

One final nit apart from a few in-line comments (most of them are just arguing with @Szelethus anyways 😛): KB vs. KiB (remember how a 2 TB hard drive appears as 1.8 TB on your machine? Because it's TiB!) is one of my pet peeves especially that Windows and most people still get it wrong nowadays (and they also teach it wrong). Can we change the default from 100 000 to 102 400? That would make it truly 100 KiB in the proper sense.

Hey @mgrang! Let's see some results on some projects and answer NoQ's comment, then I think we can put this in for all the world to see.

I have checked the results, thank you for uploading them, they look solid to me, although I'm not exactly a developer for these projects, without full understanding of what and where allocates and true path-sensitive analysis and memory modelling, they look good. (E.g. one thing this check misses I think is when the allocator returns an explicitly zero-filled memory, because that way the write without the good size is still NUL-terminated... but this requires modelling we might just not be capable of, especially not in Clang-Tidy.)

Will this properly synergise across compilers with user-specified warning options, such as -Wall -Werror?

D50353 has landed, so after a rebase this patch will not compile.

In D50488#1225064, @george.karpenkov wrote:

Why explicitly skip C projects? We would not be able to match std::X functions anyway.

Make sure you use only the C++ projects like BitCoin, LLVM/Clang, ProtoBuf and such from the Xazax CSA Test suite because this checker is not really applicable to C projects.

Ping.
Shouldn't let this thing go to waste.

@Szelethus clang-query seems to sometimes not include matcher functions that are perfectly available in the code... I recently had some issue with using isUserDefined(), it was available in my code, but clang-query always rejected it. Seems there isn't an automatic 1:1 mapping between the two.

Minor comments in-line, looking good on first glance.

Generally grammar / phrasing things that have caught my eye.

Whew, this is a big one. Generally looks good, although I would separate implementation detail functions a bit better, with perhaps more comments to move them apart a bit, it is really harsh to scroll through.

The basics of the heuristics look okay as comparing pointers from non-continuous block of memory is undefined, it would be worthy to check if no compiler warning (perhaps by specifying -W -Wall -Wextra -Weverything and various others of these enable-all flags!) is emitted if std::sort is instantiated for such a use case.

Softly pinging this. Perhaps we could discuss this and get it in before Clang7 rides off into the sunset?

Akin to D45094, pinging this too. 🙂

Pinging this as the talk has stalled.

Ah, and the function names in the test files have been made more logical.

In general, make sure the documentation page renders well in a browser.

Two minor comments.

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

@george.karpenkov @NoQ bugprone. as a category sounds nice. It also nicely corresponds to the Clang-Tidy bugprone- category. It would not be nice to further fragment the "top levels" of checker categories.

There is something that came up in my mind:

In D45532#1068700, @Szelethus wrote:

In D45532#1068647, @dkrupp wrote:

This bug report also mentions assignment operator. But for that a warning may be not so useful. In that case the members of the assigned to object should have some initialized value already which the programmer may not want to overwrite in the assignment operator.

I believe there's a checker for that already, but I'm really not sure whether UndefinedAssignmentChecker covers all such cases.

@NoQ The problem with emitting notes as events is that we lose the information that the node was a note. How does Xcode behave with these notes? Does it ignore them, or can read them from the command-line output of the analyser?

Sorry, one comment has gone missing meanwhile, I'm still getting used to this interface and hit Submit early.

@NoQ Do you reckon these tests files are too long? Perhaps the one about this inheritance, that inheritance, diamond inheritance, etc. could be split into multiple files.

Added comments on what nullptr means at call sites.

It is also std-out (llvm::outs()) in case of nullptr and not std-err.

Update to be in line with contents in dependency patch.

Simplify the patch.

• Use an even more explicit way with the documentation requiring that the file system should be an overlay.
• Add a method to easily overlay a FileSystem above the real one.

@alexfh I have updated the patch. I don't have commit rights, so if you think this is good to go, could you please commit for me?

• Overload removed, now only one CreateASTDumper function remains.
• Updated the call sites of this function to use this call.

Works for me but I haven't any sayings in these. 😇