Thanks for the fix! Looks good to me!

In D71612#1828059, @NoQ wrote:

This looks fantastic. Let's enable by it default!

• Move the setting of parameters of FunctionProtoTypeLoc to TypeLocImporter
• Add tests for TypeLoc import in case of functions

In D71612#1812476, @NoQ wrote:

In D71612#1812036, @martong wrote:

In D71612#1790045, @NoQ wrote:

I wonder if this checker will find any misuses of placement into llvm::TrailingObjects.

I've evaluated this checker on LLVM/Clang source and it did not find any results, though there are many placement new calls there. I think this is good, because there are no false positives.

In such cases it's usually a good idea to verify that the checker works correctly by artificially injecting a bug into the codebase. If the bug is not found, the checker is probably not working. If the bug is found, change it to be more and more realistic, so that to see what limitations does the checker have in terms of false negatives. Monitor analyzer stats closely (max-nodes limits, block count limits, inlining limits) in order to see what exactly goes wrong (or debug on the Exploded Graph as usual, depending on how it goes wrong). This exercise often uncovers interesting omissions :)

Can we enable the check by default then? What pieces are missing? Like, the symbolic extent/offset case is not a must. I think we have everything except maybe some deeper testing. I'd rather spend some time on the above exercise, but then enable by default if it goes well.

Related commit that moves to alpha:
https://github.com/llvm/llvm-project/commit/13ec473b9d4bd4f7a558272932b7c0806171c666

In D71612#1814225, @NoQ wrote:

In D71612#1813937, @martong wrote:

I am going to execute the analysis on LLVM/Clang as you suggested, so then we'll see what the experiment will bring us. And then we could enable it by default I think. BTW how can we do that? I could not find any default enablement related config in Checkers.td ...

Enabling checkers by default is controlled by the messy code in RenderAnalyzerOptions() in the Driver.

Wait, you already made it on-by-default. Checkers that are currently under development go into the alpha package.

• Declare ElementCountNL in if stmt
• Use makeArrayIndex instead makeIntVal

Can we enable the check by default then? What pieces are missing? Like, the symbolic extent/offset case is not a must. I think we have everything except maybe some deeper testing. I'd rather spend some time on the above exercise, but then enable by default if it goes well.

In D71612#1790045, @NoQ wrote:

I wonder if this checker will find any misuses of placement into llvm::TrailingObjects.

• Add documentation to checkers.rst
• Pass CheckerContext as last param
• Use BugType and default member initializer

• Better handling of unknown values

• Bugtype by value
• Decompose Place to base region and offset
• Add test for type hierarchy
• Remove State check
• Return directly with the size of the type in case of non-array new
• Use Optional for ElementCount
• Update warning message

Thank you guys for the assiduous review!

Recommited in bc5b7e21e32 . I changed llvm:is_contained to a simple for loop over the lookup result. This way the copy assignment of the iterator is avoided even if windows STL is used.

I'm curious what you think should happen to the clang-import-test. We could either rewrite the tests as unit tests in the ASTImporterTest you guys are already using or we move the necessary parts of the ExternalASTMerger into the clang-import-test

Simplify getRecordDeclOfFriend() further.

Addressing Alexeis comments.

Are you going to add any tests in the following patches?

I don't have any tests for now, but in a later patch we can create them.

In D70819#1781678, @martong wrote:

In D70819#1777770, @shafik wrote:

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Thanks for these cases! I am going to write unit tests for these as well.

In D70819#1777770, @shafik wrote:

Apologies for wacky C++ code that follows but will this also work for the following cases:

auto f2() { 
  auto l = []() {
      struct X{};
      return X();
  };
 return l(); 
 }

 auto f3() { 
  if ( struct X{} x; true) 
      return X();
  else return X();    
 }

 auto f4() {
    for(struct X{} x;;)
       return X();
 }

 auto f5() {
    switch(struct X{} x; 10) {
      case 10:
       return X();
    }
 }

godbolt live example

Just one more thing, maybe that is too overkill, but I think on a long term we could benefit from a unittest for this case. You could create a test similar to LLDBLookupTest in ASTImporterTest.cpp. In that Fixture we use Minimal import and the regular lookup (that is the exact case in LLDB).
In the test itself we could call ImportDefinition on a struct that has fields with record types. And then we could assert that all field's type's have complete types.

Thanks for the patch! It look almost good to me, but I have a comment about the error handling.

Put back the menu, add "open source" property.

In D70439#1772168, @Charusso wrote:

I would change the order of CCh and scan-build because we usually list stuff in alphabetical order. Also the chronological order is that, the newest is the first.

Thanks for addressing this, I just have some minor comments.

Addressing Artem's review comments.
Artem, thanks for the review!

IMPORT -> IMPORT_TYPE_LOC

Ping

@NoQ Ping.

LGTM, but wait for others please.

Alexei, thanks for the assiduous review!

• Address Alexei's comments

@Szelethus, Kristof thanks for the review.
@NoQ Ping. I'd like to have an approve from somebody who is outside the CodeChecker/E/// gang.

Ping @shafik @balazske

LGTM! Thanks!

In D68634#1701192, @martong wrote:

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

I was thinking about that too, but it turned out to be a way more intrusive change.

We have to work with the most recent existing decl (PrevDecl) of the FunctionDecl whose attribute we currently import.
We can get the PrevDecl only with the help of the lookup. We can't get it from the ToD param of InitializeImportedDecl because by the time when we call InitializeImportedDecl the redecl chain is not connected. So, we should pass PrevDecl then into GetImportedOrCreateDecl. It would require to change all call expressions of GetImportedOrCreateDecl (58 occurences). Besides, we would use the PrevDecl only in case of inheritable attributes, only they require this special care. Note that there are a bunch of non-inheritable attributes which are already handled correctly in InitializeImportedDecl.

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

LGTM!

With [modern-type-lookup], we completely evade the use of ASTImporterDelegate? That would be a wonderful thing to use only the the ExternalASTMerger on a long term...

Raphael, thanks for working on this. Overall, the changes look good to me, but please see my comment for the constructor.

2nd attempt to fix the windows build errors: 372646

Trying to fix in svn commit 372633.