[PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

In http://reviews.llvm.org/D10305#258671, @zaks.anna wrote:

> > Generating mangled names requires ASTContext which is not available during the error reporting. BugReporter does have the ASTContext, so it would not 
>
> >  be a big change to add it to the DiagnosticConsumers though. And I think the mangled name might contain too much redundant information. The only 
>
> >  relevant information here is the fully qualified name and the parts of the signature that can be ocerloaded on e.g.: constness. Using this method might also 
>
> >  be easier to debug, since the IssueString is more readable.
>
>
> I think it'd be fine to pass ASTContext to the DiagnosticConsumers. It would be useful to find out exactly what extra information the mangled names have that we do not want to see in the issue_hash.


I looked into the name mangling and found the following downsides:

- The name mangling is different when clang is running in msvc compatible mode. If we want to consistent hashes on all platforms, we should not rely on mangled names.
- The name mangling contains the calling convention which is redundant information.
- Some attributes have effect on name mangling.
- Linkage is included in mangled name.

In general using mangled name might cause unexpected behaviour for users. E.g. a user might  not expect that making a function extern "C" changes the hashes in the function.

> One issue that I see with the current approach is that we do not differentiate methods from different classes/namespaces. Is this by design?


The implementation do differentiate. It uses qualified names whenever a name is queried, which contains all of the enclosing namespaces and classes.

> 

> 

> > I definitely agree. It would be great to have a unique identifier for checkers that is independent from the name/package. It is not a trivial problem however,

> 

> >  since we probably want to support plugins too. I can think of a similar solution like git commit ids, but I think this problem might be out of the scope of this

> 

> >  patch. But I am happy to start a discussion on the mailing list and create a new patch to solve this.

> 

> 

> Sounds good to me. I agree that this is out of scope for this patch.





http://reviews.llvm.org/D10305