[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792
   "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information">;
+def note_safe_buffer_usage_suggestions_disabled : Note<
+  "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">;
----------------
jkorous wrote:
> I like this idea!
> 
> Thinking about the questions you raised:
> > Do we really need that? Maybe we should somehow throttle it to reduce noise?
> 
> The only case I can see value in not emitting this note is when the user passes `-fno-safe-buffer-usage-suggestions`. In that case it can be argued that the user is well aware of the flag since they explicitly turned it off.
> 
> That being said I am fine with the current behavior and in either case expect we will tweak this once we get feedback from the users.
> The only case I can see value in not emitting this note is when the user passes -fno-safe-buffer-usage-suggestions. In that case it can be argued that the user is well aware of the flag since they explicitly turned it off.

Yeah fair enough. This will need some rework though, because right now `-fno-...` is purely a Driver flag, so `-cc1` invocations aren't even aware of it being passed this way. So it turns from a normal flag to a somewhat quirky flag.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2159
   Sema &S;
+  bool SuggestSuggestions;
 
----------------
t-rasmud wrote:
> Was there a reason for naming this variable SuggestSuggestions? Can this be called EmitSuggestions? I think that would make it uniform and easy to follow the code.
It means "Should I emit a note reminding the user to enable `-fsafe-buffer-usage-suggestions` if they want suggestions?", so it's quite different from `EmitSuggestions` (in fact it's the opposite). Added a comment.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2203
+    if (IsRelatedToDecl) {
+      assert(!SuggestSuggestions &&
+             "Variables blamed for unsafe buffer usage without suggestions!");
----------------
ziqingluo-90 wrote:
> nitpick: 
> I was a bit confused by the name of the variable.  My understand of 
> `!SuggestSuggestions ` here means "we are suggesting suggestions now".  
> Then what does `SuggestSuggestions ` mean?
Yeah it means "Should I emit a note reminding the user to enable `-fsafe-buffer-usage-suggestions` if they want suggestions?". Added a comment to the variable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146669/new/

https://reviews.llvm.org/D146669