Issue 4942055: Various bug fixes
|
Created:
13 years, 3 months ago by delesley Modified:
13 years, 3 months ago CC:
supertri, Ollie Wild Visibility:
Public. |
Patch Set 1 #
Total comments: 10
Patch Set 2 : Yet more bug fixes, plus regression tests #
Total comments: 27
Patch Set 3 : Style updates #
MessagesTotal messages: 8
Please add tests for all of the bugs you had to fix here. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:726: } else if (dyn_cast<CXXThisExpr>(Exp)) { Use isa<CXXThisExpr> when you're not assigning the dyn_cast result to anything. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:989: NamedDecl *D = dyn_cast<NamedDecl>(CLED); You can replace the above check with dyn_cast_or_null<NamedDecl> http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1210: if ((*PI == 0) || !VisitedBlocks.alreadySet(*PI)) Generally omit parentheses around == inside ||s. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1247: if (!FirstLoopLocation.isValid()) continue; I think it's more common to put the continue on the next line. Not sure of that though. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1264: if (const NamedDecl *ContextDecl = dyn_cast<NamedDecl>(AC.getDecl())) { LLVM usually omits braces for 1-line if bodies.
Sign in to reply to this message.
http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:726: } else if (dyn_cast<CXXThisExpr>(Exp)) { On 2011/08/24 19:56:26, Jeffrey Yasskin (google) wrote: > Use isa<CXXThisExpr> when you're not assigning the dyn_cast result to anything. Done. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:989: NamedDecl *D = dyn_cast<NamedDecl>(CLED); On 2011/08/24 19:56:26, Jeffrey Yasskin (google) wrote: > You can replace the above check with dyn_cast_or_null<NamedDecl> Done. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1210: if ((*PI == 0) || !VisitedBlocks.alreadySet(*PI)) On 2011/08/24 19:56:26, Jeffrey Yasskin (google) wrote: > Generally omit parentheses around == inside ||s. Done. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1247: if (!FirstLoopLocation.isValid()) continue; On 2011/08/24 19:56:26, Jeffrey Yasskin (google) wrote: > I think it's more common to put the continue on the next line. Not sure of that > though. Done. http://codereview.appspot.com/4942055/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1264: if (const NamedDecl *ContextDecl = dyn_cast<NamedDecl>(AC.getDecl())) { On 2011/08/24 19:56:26, Jeffrey Yasskin (google) wrote: > LLVM usually omits braces for 1-line if bodies. Done.
Sign in to reply to this message.
In the future, I'd advocate for doing things a bit more incrementally. Specifically, I'd have love to have had the CFG crasher fix+test mailed out and submitted already, as the other cases are much less likely to be hit. Essentially, it seems like this is 4 patches conflated: - CFG crasher fix - Missing getCalleeDecl fix - Missing source location fix - Stylistic tweaks I like all of the changes, just advocating for more incremental development. Either way, please go ahead and mail this patch upstream asap. =D http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (left): http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1226: } It would be good to avoid conflating style and formatting changes with bug fixes in future patches. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:623: if (Block == 0) // The CFGBlock iterator occasionally hands us a null ptr This comment seems out of place. If we're going to document this, we should document it on the CFGBlock interface. The comment needed here is an explanation of why we have to check for these *here* rather than assuming our caller has already checked for NULL pointers. Specifically, tie it back to the iterator traversal that is going to blindly try to insert pruned edges. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:634: if (Block == 0) return false; But we check for null pointers in the callsites.... I think we only need to do the checkingc on this layer above where we don't control the callsites. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1108: SourceLocation loc; s/loc/Loc http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); Just return loc here, otherwise you defeat NRVO. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1251: // This should never happen, but check anyway. If it should never happen, assert. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1269: std::string FunName; It doesn't seem reasonable to leave this empty if we don't end up finding a name. maybe "<unknown>"? Also, why shift away from StringRef? http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-analysis.cpp:384: // Regression tests for unusual method names This seems too specific for a banner... http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety-parsing.cpp (right): http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-parsing.cpp:1186: // getCalleeDecl() returns NULL in the following case: These should also have FIXMEs, both here and in the source. I'm not a huge fan of working around bugs in Clang's AST (if these are bugs). http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-parsing.cpp:1215: // The first statement in this basic block has no source location This should have a FIXME
Sign in to reply to this message.
Chandler, should Delesley send this patch for review, or just commit it once we're happy with the style, and wait for post-commit comments? http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:726: } else if (isa<CXXThisExpr>(Exp)) { Reiterating Chandler, generally don't fix the style of code you're not otherwise changing. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:987: if (D == 0) return; You already have this check on line 992, and you don't use D between here and there. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); On 2011/08/26 17:47:33, chandlerc wrote: > Just return loc here, otherwise you defeat NRVO. First time I've seen anyone worry about eliding a copy of an unsigned. ;-) As a style issue, I somewhat agree, to avoid accidentally defeating NRVO somewhere it matters. On the other hand, stylistically, I'd probably move the declaration of loc into the blocks to both make clear that the result isn't used outside of those blocks, and, if NRVO mattered, to avoid the extra copy that operator= causes over direct construction. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1269: std::string FunName; On 2011/08/26 17:47:33, chandlerc wrote: > Also, why shift away from StringRef? This isn't a shift away from StringRef, but in any case I don't see a way to get a StringRef out of ContextDecl->getDeclName().
Sign in to reply to this message.
http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (left): http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1226: } This style fix was in response to a comment that Jeffrey made on an earlier version of the same patch! Do you want me to undo it? On 2011/08/26 17:47:33, chandlerc wrote: > It would be good to avoid conflating style and formatting changes with bug fixes > in future patches. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:623: if (Block == 0) // The CFGBlock iterator occasionally hands us a null ptr On 2011/08/26 17:47:33, chandlerc wrote: > This comment seems out of place. If we're going to document this, we should > document it on the CFGBlock interface. > > The comment needed here is an explanation of why we have to check for these > *here* rather than assuming our caller has already checked for NULL pointers. > Specifically, tie it back to the iterator traversal that is going to blindly try > to insert pruned edges. Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:634: if (Block == 0) return false; On 2011/08/26 17:47:33, chandlerc wrote: > But we check for null pointers in the callsites.... I think we only need to do > the checkingc on this layer above where we don't control the callsites. Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:726: } else if (isa<CXXThisExpr>(Exp)) { This isn't just a style change -- it fixes a compiler warning that we missed earlier. On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: > Reiterating Chandler, generally don't fix the style of code you're not otherwise > changing. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:987: if (D == 0) return; On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: > You already have this check on line 992, and you don't use D between here and > there. Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1108: SourceLocation loc; On 2011/08/26 17:47:33, chandlerc wrote: > s/loc/Loc Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); On 2011/08/26 17:47:33, chandlerc wrote: > Just return loc here, otherwise you defeat NRVO. Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); I'm going with Chandler this time, because it seems slightly cleaner in this particular case. On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: > On 2011/08/26 17:47:33, chandlerc wrote: > > Just return loc here, otherwise you defeat NRVO. > > First time I've seen anyone worry about eliding a copy of an unsigned. ;-) > > As a style issue, I somewhat agree, to avoid accidentally defeating NRVO > somewhere it matters. > > On the other hand, stylistically, I'd probably move the declaration of loc into > the blocks to both make clear that the result isn't used outside of those > blocks, and, if NRVO mattered, to avoid the extra copy that operator= causes > over direct construction. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1251: // This should never happen, but check anyway. On 2011/08/26 17:47:33, chandlerc wrote: > If it should never happen, assert. Done. http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1269: std::string FunName; Done. BTW, I'm calling a different API call which uses std::string, not StringRef. On 2011/08/26 17:47:33, chandlerc wrote: > It doesn't seem reasonable to leave this empty if we don't end up finding a > name. maybe "<unknown>"? Also, why shift away from StringRef?
Sign in to reply to this message.
(Hopefully) final version of patch. I will bring the FIXME's to the attention of other reviewers when I post upstream. -DeLesley http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-analysis.cpp:384: // Regression tests for unusual method names But it doesn't fit in the other sections... On 2011/08/26 17:47:33, chandlerc wrote: > This seems too specific for a banner... http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... File test/SemaCXX/warn-thread-safety-parsing.cpp (right): http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-parsing.cpp:1186: // getCalleeDecl() returns NULL in the following case: On 2011/08/26 17:47:33, chandlerc wrote: > These should also have FIXMEs, both here and in the source. > > I'm not a huge fan of working around bugs in Clang's AST (if these are bugs). Done. http://codereview.appspot.com/4942055/diff/4001/test/SemaCXX/warn-thread-safe... test/SemaCXX/warn-thread-safety-parsing.cpp:1215: // The first statement in this basic block has no source location On 2011/08/26 17:47:33, chandlerc wrote: > This should have a FIXME Done.
Sign in to reply to this message.
Sorry for my inability to read in a couple places in my comments here. :) Generally you should upload the new version of a patch before mailing out your replies, but assuming you only changed things mentioned here, LGTM now. On Fri, Aug 26, 2011 at 11:40 AM, <delesley@google.com> wrote: > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > File lib/Sema/AnalysisBasedWarnings.cpp (left): > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1226: } > This style fix was in response to a comment that Jeffrey made on an > earlier version of the same patch! Do you want me to undo it? > > On 2011/08/26 17:47:33, chandlerc wrote: >> >> It would be good to avoid conflating style and formatting changes with > > bug fixes >> >> in future patches. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > File lib/Sema/AnalysisBasedWarnings.cpp (right): > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:623: if (Block == 0) // The CFGBlock > iterator occasionally hands us a null ptr > On 2011/08/26 17:47:33, chandlerc wrote: >> >> This comment seems out of place. If we're going to document this, we > > should >> >> document it on the CFGBlock interface. > >> The comment needed here is an explanation of why we have to check for > > these >> >> *here* rather than assuming our caller has already checked for NULL > > pointers. >> >> Specifically, tie it back to the iterator traversal that is going to > > blindly try >> >> to insert pruned edges. > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:634: if (Block == 0) return false; > On 2011/08/26 17:47:33, chandlerc wrote: >> >> But we check for null pointers in the callsites.... I think we only > > need to do >> >> the checkingc on this layer above where we don't control the > > callsites. > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:726: } else if > (isa<CXXThisExpr>(Exp)) { > This isn't just a style change -- it fixes a compiler warning that we > missed earlier. > > On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: >> >> Reiterating Chandler, generally don't fix the style of code you're not > > otherwise >> >> changing. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:987: if (D == 0) return; > On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: >> >> You already have this check on line 992, and you don't use D between > > here and >> >> there. > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1108: SourceLocation loc; > On 2011/08/26 17:47:33, chandlerc wrote: >> >> s/loc/Loc > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); > On 2011/08/26 17:47:33, chandlerc wrote: >> >> Just return loc here, otherwise you defeat NRVO. > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1120: return SourceLocation(); > I'm going with Chandler this time, because it seems slightly cleaner in > this particular case. > > On 2011/08/26 18:08:49, Jeffrey Yasskin wrote: >> >> On 2011/08/26 17:47:33, chandlerc wrote: >> > Just return loc here, otherwise you defeat NRVO. > >> First time I've seen anyone worry about eliding a copy of an unsigned. > > ;-) > >> As a style issue, I somewhat agree, to avoid accidentally defeating > > NRVO >> >> somewhere it matters. > >> On the other hand, stylistically, I'd probably move the declaration of > > loc into >> >> the blocks to both make clear that the result isn't used outside of > > those >> >> blocks, and, if NRVO mattered, to avoid the extra copy that operator= > > causes >> >> over direct construction. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1251: // This should never happen, > but check anyway. > On 2011/08/26 17:47:33, chandlerc wrote: >> >> If it should never happen, assert. > > Done. > > http://codereview.appspot.com/4942055/diff/4001/lib/Sema/AnalysisBasedWarning... > lib/Sema/AnalysisBasedWarnings.cpp:1269: std::string FunName; > Done. BTW, I'm calling a different API call which uses std::string, not > StringRef. > > On 2011/08/26 17:47:33, chandlerc wrote: >> >> It doesn't seem reasonable to leave this empty if we don't end up > > finding a >> >> name. maybe "<unknown>"? Also, why shift away from StringRef? > > http://codereview.appspot.com/4942055/ >
Sign in to reply to this message.
On Fri, Aug 26, 2011 at 11:45 AM, Jeffrey Yasskin <jyasskin@google.com>wrote: > Sorry for my inability to read in a couple places in my comments here. :) > > Generally you should upload the new version of a patch before mailing > out your replies, but assuming you only changed things mentioned here, > LGTM now. > FYI, this still needs to be mailed upstream. That's the important step I'm waiting for.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
