aaron.ballman added a comment. In D59467#1443173 <https://reviews.llvm.org/D59467#1443173>, @Tyker wrote:
> @lebedev.ri where are tests for AST serialization are located ? i didn't find > them. They live in clang\tests\PCH. In D59467#1440608 <https://reviews.llvm.org/D59467#1440608>, @Tyker wrote: > i implemented the semantic the changes for if for, while and do while > statement and the AST change to if. can you review it and tell me if it is > fine so i implement the rest. i didn't update the test so they will fail. I think this may be moving closer to the correct direction now, but I'm still curious to hear if @rsmith has similar opinions. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164 +def warn_no_associated_branch : Warning< + "attribute %0 not assiciated to any branch is ignored">, InGroup<IgnoredAttributes>; +def warn_conflicting_attribute : Warning< ---------------- Typo: associated Should probably read "attribute %0 is not associated with a branch and is ignored" Also, I'd rename the diagnostic to be `warn_no_likelihood_attr_associated_branch` ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165-8166 + "attribute %0 not assiciated to any branch is ignored">, InGroup<IgnoredAttributes>; +def warn_conflicting_attribute : Warning< + "%0 contradicing with previous %1 attribute">, InGroup<IgnoredAttributes>; + ---------------- I'd rename the diagnostic to `warn_conflicting_likelihood_attrs` and reword the diagnostic to "attribute %0 conflicts with previous attribute %1" ================ Comment at: clang/include/clang/Sema/Scope.h:163 + /// BranchAttr - Likelihood attribute associated with this Branch or nullptr + Attr *BranchAttr; ---------------- Missing full stop at the end of the comment. You should check all the comments in the patch to be sure they are full sentences (start with a capital letter, end with punctuation, are grammatically correct, etc). ================ Comment at: clang/include/clang/Sema/Scope.h:164 + /// BranchAttr - Likelihood attribute associated with this Branch or nullptr + Attr *BranchAttr; + ---------------- Rather than store an `Attr` here, would it make sense to instead store a `LikelihoodAttr` directly? That may clean up some casts in the code. ================ Comment at: clang/include/clang/Sema/Sema.h:3845 + /// emit diagnostics and determinate the hint for and If statement + BranchHint HandleIfStmtHint(Stmt *thenStmt, Stmt *elseStmt, Attr *ThenAttr, + Attr *ElseAttr); ---------------- You should check all of the identifiers introduced by the patch to ensure they meet our coding style requirements (https://llvm.org/docs/CodingStandards.html). `thenStmt` -> `ThenStmt`, etc. ================ Comment at: clang/lib/Sema/SemaStmt.cpp:526 + Attr *ThenAttr, Attr *ElseAttr) { + // diagnose branch with attribute and null statement as empty body + if (thenStmt && isa<AttributedStmt>(thenStmt) && ---------------- The condition here is incorrect -- it's not checking what kind of attributed statement is present to see if it's a likelihood statement. However, what is the value in diagnosing this construct? It seems not-entirely-unreasonable for a user to write, for instance: ``` if (foo) { [[likely]]; ... } else [[unlikely]]; ``` ================ Comment at: clang/lib/Sema/SemaStmt.cpp:548 + } else { + if (ThenAttr->getSpelling()[0] == 'l') + Hint = Taken; ---------------- This is fragile -- you should do something more like: ``` if (const auto *LA = dyn_cast<LikelihoodAttr>(ThenAttr)) { Hint = LA->isLikely() ? ... : ...; } ``` You'll want to add an accessor onto the LikelihoodAttr object in Attr.td to implement the `isLikely()` (and `isUnlikely()`) functions based on the attribute spelling. There are examples of this in Attr.td. ================ Comment at: clang/lib/Sema/SemaStmtAttr.cpp:66-73 + if (!ControlScope || + !(ControlScope->getFlags() & Scope::ControlScope || + ControlScope->getFlags() & Scope::BreakScope) || + ControlScope->getFlags() & Scope::SEHExceptScope || + ControlScope->getFlags() & Scope::SEHTryScope || + ControlScope->getFlags() & Scope::TryScope || + ControlScope->getFlags() & Scope::FnTryCatchScope) { ---------------- This doesn't seem to account for `switch` statements, like this, does it? ``` switch (foo) { [[likely]] case 1: break; [[unlikely]] case 2: break; [[likely]] case 3: break; [[unlikely]] default: break; } ``` Note, it's perfectly reasonable for there to be repeated attributes here because some cases may be more likely than others. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
