aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Adding Richard for design strategy discussion.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164 +def err_must_appear_after_branch : Error< + "%0 can only appear after a selection or iteration statement">; +def warn_attribute_already_present : Warning< ---------------- riccibruno wrote: > I don't think that this is right. I don't see this restriction in the > specification. A warning should definitely be emitted when the attribute is > ignored, but I believe that an error is inappropriate. > I don't think that this is right. I don't see this restriction in the > specification. A warning should definitely be emitted when the attribute is > ignored, but I believe that an error is inappropriate. Agreed. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165 + "%0 can only appear after a selection or iteration statement">; +def warn_attribute_already_present : Warning< + "was already marked %0">; ---------------- This diagnostic isn't needed -- `warn_duplicate_attribute_exact` will cover it. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8167 + "was already marked %0">; +def err_mutuably_exclusive_likelihood : Error< + "%0 and %1 are mutually exclusive">; ---------------- This diagnostic is not needed -- `err_attributes_are_not_compatible` will cover it. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8170 +def err_contradictory_attribute : Warning< + "%0 contradicing with previous attribute">; + ---------------- riccibruno wrote: > This should be `warn_contradictory_attribute`, and I am not seeing tests for > this. This note also isn't needed. You should use `note_conflicting_attribute` instead. ================ Comment at: clang/lib/Sema/SemaStmtAttr.cpp:64-75 + if (CurScope) { + Scope* ControlScope = CurScope->getParent(); + if (!ControlScope || + !(ControlScope->getFlags() & Scope::ControlScope || + ControlScope->getFlags() & Scope::BreakScope) || + CurScope->getFlags() & Scope::CompoundStmtScope || + ControlScope->getFlags() & Scope::SEHExceptScope || ---------------- This is incorrect -- nothing in the standard requires the attribute to appear after a branch statement. I'm not even 100% convinced that this should map directly to `__builtin_expect` in all cases (though it certainly would for conditional branches), because that doesn't help with things like catch handlers or labels. ================ Comment at: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp:52 +} \ No newline at end of file ---------------- Be sure to add the newline, please. 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