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