Mordante marked 2 inline comments as done.
Mordante added a comment.
In D89210#2332143 <https://reviews.llvm.org/D89210#2332143>, @aaron.ballman
wrote:
> LGTM aside from a documentation request, but you may want to wait a few days
> before committing in case Richard or John have opinions.
Thanks for the review!
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1815
+ switch(i) {
+ [[likely]] case 1: // This value is likely
----------------
aaron.ballman wrote:
> Can you add a second example that shows what to expect from fallthrough
> behavior? Perhaps something like:
> ```
> switch (i) {
> [[likely]] case 0: // This value and code path is likely
> ...
> [[fallthrough]];
> case 1: // No likelihood attribute, code path is likely due to fallthrough
> break;
> case 2: // No likelihood attribute, code path is neutral
> [[fallthrough]];
> [[unlikely]] default: // This value and code path are both unlikely
> break;
> }
> ```
I've added this example with minor modifications. Note `case 1` is neutral
since falling through has no effect.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:375
+ ArrayRef<const Attr *> Attrs) {
+ // clang-format off
switch (S->getStmtClass()) {
----------------
aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > How ugly is the default clang-format formatting for this? If it's not too
> > > bad, perhaps we just re-format the switch statement as a whole?
> > It looks fine default formatted, I just thought we wanted to keep it
> > compact. But I've no problem with keeping the code default formatted.
> I tend to prefer whatever the default formatting is just so we don't have
> formatting comments all over the place (but I'm fine if the unique formatting
> is important for code readability).
I'll keep that in mind.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89210/new/
https://reviews.llvm.org/D89210
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
