[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 29 13:55:54 PDT 2021
rjmccall added a comment.
Seems conceptually okay, given that we have an option to control it.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:796
+ bool EmitBoolCondBranch = !C || !C->isOne();
+ bool CondIsConst = C;
const SourceRange &R = S.getSourceRange();
----------------
fhahn wrote:
> lebedev.ri wrote:
> > I think, if we really want to give this a name, perhaps we want something even more specific,
> > perhaps `CondIsConstImm`/`CondIsConstInt`?
> I updated the name to `CondIsConstInt`
Please use an explicit `!= nullptr` check when converting a pointer to `bool` outside of an obvious boolean context.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:535-536
+ // Hence each function is 'mustprogress' in C++11 or later.
return getLangOpts().CPlusPlus11 || getLangOpts().CPlusPlus14 ||
getLangOpts().CPlusPlus17 || getLangOpts().CPlusPlus20;
}
----------------
fhahn wrote:
> lebedev.ri wrote:
> > Since every new standard version will have to be added here,
> > can we invert this and just check for the know fixed set
> > of the versions where this doesn't apply?
> I tried, but it appears as if there's no LangOpt for `C++98`. (this also seems not directly related to the patch, so I guess we could do that as follow-up as well?)
The way these options work is that the later standards imply the early standards; that's why there isn't a `CPlusPlus98`. You just need `CPlusPlus11` here.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:562
+ // Loops with non-constant conditions must make progress in C11 and later.
+ return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x;
}
----------------
fhahn wrote:
> lebedev.ri wrote:
> > Since every new standard version will have to be added here,
> > can we invert this and just check for the know fixed set
> > of the versions where this doesn't apply?
> I tried but I am not sure how to best do that, given there's no explicit C89 lang opt.
Same thing as with the C++ language versions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96418/new/
https://reviews.llvm.org/D96418
More information about the cfe-commits
mailing list