[PATCH] D91979: [Clang][Attr] Introduce the `assume` function attribute

[Aaron Ballman via Phabricator via llvm-commits]

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