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