[PATCH] D12379: Fix the bugs in the mapDiagnosticRanges (Still in progress)

[Richard Trieu via cfe-commits]

rtrieu added inline comments.

================
Comment at: lib/Basic/SourceManager.cpp:1008
@@ -1008,1 +1007,3 @@
+bool SourceManager::isMacroArgExpansion(SourceLocation Loc, 
+                                      SourceLocation *NewLoc) const {
   if (!Loc.isMacroID()) return false;
----------------
Fix alignment so function arguments line up.

================
Comment at: lib/Basic/SourceManager.cpp:1013
@@ -1011,2 +1012,3 @@
   const SrcMgr::ExpansionInfo &Expansion = getSLocEntry(FID).getExpansion();
-  return Expansion.isMacroArgExpansion();
+  if (!Expansion.isMacroArgExpansion()) return false;
+  if (NewLoc)
----------------
Add line break after this line.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:323
@@ +322,3 @@
+  else
+    Backup = SM->getImmediateSpellingLoc(Backup);
+
----------------
You did not add that here.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:331
@@ +330,3 @@
+  return retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM);
+}
+
----------------
Using reference parameters to input/output data into functions is confusing and generally not done.  SourceLocation is a small enough class that it is commonly used as return values and it has a built-in invalid value.  Use the following function:
```
static SourceLocation retrieveMacroLocation(SourceLocation Loc,
                                            FileID MacroFileID,
                                            FileID CaretFileID,
                                            bool getBeginLoc,
                                            const SourceManager *SM) {
  if (MacroFileID == CaretFileID) return Loc;
  if (!Loc.isMacroID()) return SourceLocation();

  SourceLocation MacroLocation, MacroArgLocation;

  if (SM->isMacroArgExpansion(Loc)) {
    MacroLocation = SM->getImmediateMacroCallerLoc(Loc);
    MacroArgLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
                                   : SM->getImmediateExpansionRange(Loc).second;
  } else {
    MacroLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
                                : SM->getImmediateExpansionRange(Loc).second;
    MacroArgLocation = SM->getImmediateSpellingLoc(Loc);
  }

  MacroFileID = SM->getFileID(MacroLocation);
  MacroLocation = retrieveMacroLocation(MacroLocation, MacroFileID, CaretFileID,
                                        getBeginLoc, SM);
  if (MacroLocation.isValid()) return MacroLocation;

  MacroFileID = SM->getFileID(MacroArgLocation);
  return retrieveMacroLocation(MacroArgLocation, MacroFileID, CaretFileID,
                               getBeginLoc, SM);
}
```

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:352-367
@@ -309,12 +351,18 @@
+
+  End = Backup;
+  EndFileID = SM->getFileID(Backup);
+  return retrieveEndLocation(End,EndFileID,CaretLocFileID,SM);
+}
+
 // Helper function to fix up source ranges.  It takes in an array of ranges,
 // and outputs an array of ranges where we want to draw the range highlighting
 // around the location specified by CaretLoc.
 //
 // To find locations which correspond to the caret, we crawl the macro caller
 // chain for the beginning and end of each range.  If the caret location
 // is in a macro expansion, we search each chain for a location
 // in the same expansion as the caret; otherwise, we crawl to the top of
 // each chain. Two locations are part of the same macro expansion
 // iff the FileID is the same.
 static void mapDiagnosticRanges(
     SourceLocation CaretLoc,
----------------
It is more important to have good comments than debug functions since that saves the trouble of running a debugger.  If someone is debugging this code, this function would be in the wrong place.  Instead, the SourceManager would be a better place to put it, and it saves the trouble of passing in a SourceManager object.  Besides that, there special considerations for debugging functions, like annotating it properly so it only shows up in debug builds and not increase the size of regular builds.  If you still feel it is important, then the work should go into another patch.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:406-407
@@ -374,1 +405,4 @@
+    // Do the backtracking.
+    if (!retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM)) continue;
+    if (!retrieveEndLocation(End,EndFileID,CaretLocFileID,SM)) continue;
 
----------------
Use:
```
    Begin = retrieveMacroLocation(Begin, BeginFileID, CaretLocFileID,
                                  true /*getBeginLoc*/, SM);
    End = retrieveMacroLocation(End, BeginFileID, CaretLocFileID,
                                false /*getBeginLoc*/, SM);
    if (Begin.isInvalid() || End.isInvalid()) continue;
```
with the fixed function above.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:455
@@ -419,1 +454,3 @@
 
+static bool checkLocForMacroArgExpansion(SourceLocation Loc,
+                                         const SourceManager &SM,
----------------
zhengkai wrote:
> Because the function isMacroArgExpansion doesn't check if all locations are in the same argument location,
I still don't know what you are doing here.  You have two SourceLocation's, Loc and PreLoc.  Those are not good names for telling them apart or what they do.  You pass the second SourceLocation in by reference, both as an input value, and later as something to put an output value in.  This is a bad idea.  If you need a SourceLocation to be passed back to the caller, make the function return a SourceLocation, or use a SourceLocation pointer as an argument that is only for output.  The SourceLocation class is tiny so it is okay to copy around.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:460
@@ +459,3 @@
+  if (SM.isMacroArgExpansion(Loc, &NewLoc)) {
+    if (PreLoc.isInvalid() || NewLoc == SourceLocation() 
+                           || PreLoc == NewLoc) {
----------------
Do the same for NewLoc

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:471
@@ -422,1 +470,3 @@
+                                           const SourceManager &SM,
+                                           SourceLocation &Loc) {
   SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
----------------
What is this SourceLocation?  Why is it passed by reference?

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:502
@@ +501,3 @@
+  /// To store the source location of the argument location.
+  SourceLocation ArgumentLoc = SourceLocation(); 
+
----------------
Not done.

================
Comment at: test/Misc/caret-diags-macros.c:210
@@ -215,3 +209,3 @@
 void f(char* pMsgBuf, char* pKeepBuf) {
-Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", Cstrlen(pKeepBuf));
+Csprintf(pMsgBuf,"\nEnter minimum anagram length (2-%1d): ", strlen_test(pKeepBuf));
 }
----------------
Add a FIXME comment that it should be changed back to Cstrlen when the bug is resolved.


http://reviews.llvm.org/D12379