[PATCH] D12379: Fix the bugs in the mapDiagnosticRanges (Still in progress)
Richard Trieu via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 15 18:20:28 PDT 2015
rtrieu added inline comments.
================
Comment at: include/clang/Basic/SourceManager.h:333
@@ -332,2 +332,3 @@
+
bool isMacroBodyExpansion() const {
----------------
Remove unrelated whitespace change.
================
Comment at: include/clang/Basic/SourceManager.h:1155-1164
@@ -1153,2 +1154,12 @@
+ /// \brief Tests whether the given source location represents a macro
+ /// argument's expansion into the function-like macro definition.
+ ///
+ /// Such source locations only appear inside of the expansion
+ /// locations representing where a particular function-like macro was
+ /// expanded.
+ /// If the return value is true, the RawCode will store the raw encoding
+ /// of the source location of the expanded argument.
+ bool isMacroArgExpansion(SourceLocation Loc, SourceLocation &NewLoc) const;
+
/// \brief Tests whether the given source location represents the expansion of
----------------
Merge the two isMacroArgExpansion functions by making the second argument a SourceLocation pointer with default argument of nullptr. See isAtStartOfImmediateMacroExpansion or isAtEndOfImmediateMacroExpansion functions for how this is done.
================
Comment at: lib/Basic/SourceManager.cpp:1015-1024
@@ -1014,1 +1014,12 @@
+bool SourceManager::isMacroArgExpansion(SourceLocation Loc,
+ SourceLocation &NewLoc) const {
+ if (!Loc.isMacroID()) return false;
+
+ FileID FID = getFileID(Loc);
+ const SrcMgr::ExpansionInfo &Expansion = getSLocEntry(FID).getExpansion();
+ if (!Expansion.isMacroArgExpansion()) return false;
+ NewLoc = Expansion.getExpansionLocStart();
+ return true;
+}
+
----------------
Merge with other isMacroArgExpansion function
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:311
@@ +310,3 @@
+/// to match the \p CaretLocFileID.
+
+static bool retrieveBeginLocation(SourceLocation &Begin,
----------------
No newline between comment and function. And what is the logic behind in this function that wasn't in the code it is replacing?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:312
@@ +311,3 @@
+
+static bool retrieveBeginLocation(SourceLocation &Begin,
+ FileID &BeginFileID,
----------------
Why not return a SourceLocation instead of passing one in by reference?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:313
@@ +312,3 @@
+static bool retrieveBeginLocation(SourceLocation &Begin,
+ FileID &BeginFileID,
+ FileID CaretLocFileID,
----------------
Why is this passed by reference?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:314-350
@@ +313,39 @@
+ FileID &BeginFileID,
+ FileID CaretLocFileID,
+ const SourceManager *SM) {
+ if (BeginFileID == CaretLocFileID) return true;
+ if (!Begin.isMacroID()) return false;
+ SourceLocation Backup = Begin;
+ Begin = SM->getImmediateMacroCallerLoc(Begin);
+ if (SM->isMacroArgExpansion(Backup))
+ Backup = SM->getImmediateExpansionRange(Backup).first;
+ else
+ Backup = SM->getImmediateSpellingLoc(Backup);
+ BeginFileID = SM->getFileID(Begin);
+ if (retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM)) return true;
+ Begin = Backup;
+ BeginFileID = SM->getFileID(Backup);
+ return retrieveBeginLocation(Begin,BeginFileID,CaretLocFileID,SM);
+}
+
+static bool retrieveEndLocation(SourceLocation &End,
+ FileID &EndFileID,
+ FileID CaretLocFileID,
+ const SourceManager *SM) {
+ if (EndFileID == CaretLocFileID) return true;
+ if (!End.isMacroID()) return false;
+ SourceLocation Backup = End;
+ End = SM->getImmediateMacroCallerLoc(End);
+ if (SM->isMacroArgExpansion(Backup))
+ Backup = SM->getImmediateExpansionRange(Backup).second;
+ else {
+ End = SM->getImmediateExpansionRange(Backup).second;
+ Backup = SM->getImmediateSpellingLoc(Backup);
+ }
+ EndFileID = SM->getFileID(End);
+ if (retrieveEndLocation(End,EndFileID,CaretLocFileID,SM)) return true;
+ End = Backup;
+ EndFileID = SM->getFileID(Backup);
+ return retrieveEndLocation(End,EndFileID,CaretLocFileID,SM);
+}
+
----------------
Use more vertical whitespace when coding.
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:323
@@ +322,3 @@
+ else
+ Backup = SM->getImmediateSpellingLoc(Backup);
+ BeginFileID = SM->getFileID(Begin);
----------------
In the other function, there is "End = SM->getImmediateExpansionRange(Backup).second;" Why is there not a "Begin = SM->getImmediateExpansionRange(Backup).first;" here?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:331
@@ +330,3 @@
+
+static bool retrieveEndLocation(SourceLocation &End,
+ FileID &EndFileID,
----------------
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?
================
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,
----------------
Function not used; remove it.
================
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;
----------------
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;
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:466
@@ -419,1 +465,3 @@
+static bool checkLocForMacroArgExpansion(SourceLocation Loc,
+ const SourceManager &SM,
----------------
What is this function doing that is not captured by the old isMacroArgExpansion function?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:469
@@ +468,3 @@
+ SourceLocation &PreLoc) {
+ SourceLocation NewLoc = SourceLocation();
+ if (SM.isMacroArgExpansion(Loc, NewLoc)) {
----------------
Drop the "= SourceLocation". Default initialization of SourceLocation does the same thing.
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:471
@@ +470,3 @@
+ if (SM.isMacroArgExpansion(Loc, NewLoc)) {
+ if (PreLoc == SourceLocation() || NewLoc == SourceLocation()
+ || PreLoc == NewLoc) {
----------------
Use "PreLoc.isInvalid()" instead of "PreLoc == SourceLocation()".
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:504
@@ -441,2 +503,3 @@
- if (!SM.isMacroArgExpansion(Loc))
+ /// Count all valid ranges.
+ unsigned ValidCount = 0;
----------------
Why does the range count matter?
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:512
@@ +511,3 @@
+
+ /// To store the raw encoding of the argument location.
+ SourceLocation SameLoc = SourceLocation();
----------------
Comment is out of date, no longer holding the raw encoding. Rename the variable to ArgumentLoc to convey what the SourceLocation is holding.
================
Comment at: lib/Frontend/DiagnosticRenderer.cpp:513
@@ +512,3 @@
+ /// To store the raw encoding of the argument location.
+ SourceLocation SameLoc = SourceLocation();
+
----------------
Drop the "= SourceLocation()". Default initialization for SourceLocation does the same thing.
================
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));
}
----------------
Why is Cstrlen changed to strlen_test?
http://reviews.llvm.org/D12379
More information about the cfe-commits
mailing list