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

[Zhengkai Wu via cfe-commits]

zhengkai added inline comments.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:312
@@ +311,3 @@
+
+static bool retrieveBeginLocation(SourceLocation &Begin,
+                             FileID &BeginFileID,
----------------
rtrieu wrote:
> Why not return a SourceLocation instead of passing one in by reference?
Because this function has two recursive calls inside.
So if return a SourceLocation here, I will need to use a temporal variable to store the result of the first call.
And this doesn't make it easier.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:323
@@ +322,3 @@
+  else
+    Backup = SM->getImmediateSpellingLoc(Backup);
+  BeginFileID = SM->getFileID(Begin);
----------------
rtrieu wrote:
> In the other function, there is "End = SM->getImmediateExpansionRange(Backup).second;"  Why is there not a "Begin = SM->getImmediateExpansionRange(Backup).first;" here?
  Begin = SM->getImmediateMacroCallerLoc(Begin);
Use getImmediateMacroCallerLoc function here.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:331
@@ +330,3 @@
+
+static bool retrieveEndLocation(SourceLocation &End,
+                                FileID &EndFileID,
----------------
rtrieu wrote:
> There is a lot of duplicated code between these two functions.  Can they merged together with an option to select whether to check the start or end locations?
I tried it but the code doesn't seem easier.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:352-367
@@ -309,1 +351,18 @@
+
+/// Helper function to print out all the backtrace locations 
+/// for a source location.
+
+static void retrieveAllBacktraces(SourceLocation Loc,
+                                  const SourceManager *SM) {
+  llvm::errs() << "New level\n";
+  llvm::errs() << Loc.printToString(*SM) << " " << 
+                  SM->getFileID(Loc).getHashValue() << "\n";
+  if (!Loc.isMacroID()) return;
+  if (SM->isMacroArgExpansion(Loc)) llvm::errs() << "is Macro Arg Expansion\n";
+  llvm::errs() << "Down Spelling Loc\n";
+  retrieveAllBacktraces(SM->getImmediateSpellingLoc(Loc),SM);
+  llvm::errs() << "Down Expansion Range\n";
+  retrieveAllBacktraces(SM->getImmediateExpansionRange(Loc).first,SM);
+}
+
 // Helper function to fix up source ranges.  It takes in an array of ranges,
----------------
rtrieu wrote:
> Function not used; remove it.
deleted. But I wonder this function is important for those who want to debug this code.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:417-418
@@ -374,1 +416,4 @@
+    // Do the backtracking.
+    if (!retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM)) continue;
+    if (!retrieveEndLocation(End,EndFileID,CaretLocFileID,SM)) continue;
 
----------------
rtrieu wrote:
> Can these function be rewritten to return SourceLocation's?  Then it would be:
> Begin = retrieveBeginLocation(Begin, BeginFileID, CaretLocFileID, SM);
> End = retrieveEndLocation(End, EndFileID, CaretLocFileID, SM);
> 
> if (Begin.invalid() || End.invalid()) continue;
> 
Replied before.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:466
@@ -419,1 +465,3 @@
 
+static bool checkLocForMacroArgExpansion(SourceLocation Loc,
+                                         const SourceManager &SM,
----------------
rtrieu wrote:
> What is this function doing that is not captured by the old isMacroArgExpansion function?
I don't understand what you mean here.

================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:504
@@ -441,2 +503,3 @@
 
-  if (!SM.isMacroArgExpansion(Loc))
+  /// Count all valid ranges.
+  unsigned ValidCount = 0;
----------------
rtrieu wrote:
> Why does the range count matter?
Because some ranges passed in are invalid.
And if the mapDiagnosticRanges function drops some ranges, this means this macro call doesn't contain all the information needed of all the ranges.

================
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));
 }
----------------
rtrieu wrote:
> Why is Cstrlen changed to strlen_test?
The preprocessor can't get right when the argument itself is a function-like macro. This bug is not due to the high-level macro case but due to this problem.


http://reviews.llvm.org/D12379