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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
