[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso requested changes to this revision.
Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1985
     bool partOfParentMacro = false;
+    StringRef PName = "";
     if (ParentEx->getBeginLoc().isMacroID()) {
----------------
`ParentName`?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1996
+      StringRef MacroName = StartName;
+      while (true) {
         LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
----------------
I was never cool in algorithms, but I think it is over-complicated. In Hungary we are about to build a new nuclear power plant so here it is really emphasized: never-ever create an infinite loop.
What about the following?:
```
      while (LocStart.isMacroID()) {                                             
        StringRef CurrentMacroName = Lexer::getImmediateMacroNameForDiagnostics( 
            LocStart, BRC.getSourceManager(),                                    
            BRC.getASTContext().getLangOpts());                                  
        LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);  
        if (CurrentMacroName != ParentName)                                           
          MacroName = CurrentMacroName;                                          
      }
```


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2003
+            LocStart, BRC.getSourceManager(),
+            BRC.getASTContext().getLangOpts());
+        if (NextMacroName == PName)
----------------
There is too much duplication and it is difficult to understand what is going on. May here is the time for deduplicating?
Please note that, there is a function called `getMacroName(SourceLocation, BugReporterContext &)`, may you would put your own helper-function above that.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121