aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:742 "the argument to %0 has side effects that will be discarded">, - InGroup<DiagGroup<"assume">>; + InGroup<Assumption>; +def warn_assume_attribute_string_unknown : Warning< ---------------- aaron.ballman wrote: > This changes behavior for users because `-Wno-assume` now needs to be spelled > `-Wno-assumption` -- I think we may want to keep the diagnostics for the > assumption attribute separate from the diagnostics for builtin assume. > > For instance, I think the assumption attribute warnings will all wind up > leading to an ignored attribute, so perhaps the new grouping should be under > the existing ignored attributes group. I think we can ignore this comment now, but I do wonder whether we should split the diagnostic group somewhat. To me, the "may be misspelled" warning is going to always be a serious warning -- it means the user fat-fingered something and the attribute is likely to not do anything useful. But the non-misspelling warning seems like a less severe warning because I may be relying on an undocumented string from a pass. So I could imagine wanting to turn off all of the warnings for using an undocumented string from a pass but wanting to retain all of the misspelling warnings. However, you may be envisioning a different usage pattern for this than I am. WDYT? ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:744 +def warn_assume_attribute_string_unknown : Warning< + "the assumption string '%0' is not known and might be misspelled">, + InGroup<Assumption>; ---------------- aaron.ballman wrote: > I'd say that this case should probably be worded `unknown assumption string > '%0'; attribute ignored` In light of wanting to keep the attribute around, I think this should be `unknown assumption string '%0'; attribute is potentially ignored` or something along those lines. Basically, I think the diagnostic should focus on the fact that the user cannot predict what happens with this attribute. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:747 +def warn_assume_attribute_string_unknown_suggested : Warning< + "the assumption string '%0' is not known and might be misspelled; " + "did you mean '%1'?">, ---------------- aaron.ballman wrote: > Similarly: `unknown assumption string '%0' may be misspelled; attribute > ignored, did you mean '%1'?` Similar here. `unknown assumption string '%0' may be misspelled; attribute is potentially ignored, did you mean '%1'?` ================ Comment at: clang/include/clang/Sema/Sema.h:10002 + /// Check if \p AssumptionStr is a known assumption and warn if not. + void CheckAssumptionAttr(SourceLocation Loc, StringRef AssumptionStr); + ---------------- Should this be a static function in SemaDeclAttr.cpp rather than a public part of the Sema interface? I can't think of any other callers for this aside from `handleAssumptionAttr()`. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1681 + StringRef Str; + if (AL.getNumArgs() == 1 && !S.checkStringLiteralArgumentAttr(AL, 0, Str)) + return; ---------------- jdoerfert wrote: > aaron.ballman wrote: > > No need to check the count manually, the common attribute handler should do > > that for you already. > Copied from the generic templated handler. Ah, yeah, that one needs to do the test because it may be called for an attribute expecting a variadic number of arguments. ================ Comment at: llvm/include/llvm/IR/Assumptions.h:15 + +#ifndef LLVM_ASSUMPTIONS_H +#define LLVM_ASSUMPTIONS_H ---------------- Should probably fix the lint warning here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91979/new/ https://reviews.llvm.org/D91979 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
