This is an archive of the discontinued LLVM Phabricator instance.

Change where we handle arg-dependent diagnose_if attributes
ClosedPublic

Authored by george.burgess.iv on Jan 18 2017, 8:35 PM.

Details

Summary

As it turns out, emitting diagnostics from places where you're not meant to emit them from is a very bad idea. :)

After some looking around, it seems that it's less insane to check for diagnose_if attributes in code that's already checking for e.g. nullness warnings. As a result, we get to rip out the diagnose_if-induced changes to overloading. Woohoo!

This also includes a slight change to how diagnose_if works: for "error" calls, we'll always assume that overload resolution chose the overload the user wanted. So, regardless of whetehr a diagnose_if attribute emits a diag or not, we'll continue building the AST/etc as usual.

This patch aims to fix PR31638, PR31639, and PR31640.

Diff Detail

Event Timeline

Sprinkle in a few consts, use const auto * in range for loops.

rsmith added inline comments.Jan 23 2017, 11:31 AM
include/clang/Sema/Overload.h
758

Typo "alloates"

lib/Sema/SemaChecking.cpp
2493

Can this be moved inside checkCall?

george.burgess.iv marked 2 inline comments as done.

Addressed all feedback.

Richard noted that, because we're now doing these checks after overload resolution has occurred, we no longer need to convert arguments for arg-dependent diagnose_if checks. As a result, we get to make lots of things const. Yay!

Also, I plan to submit this (once it's LGTM'ed) to the 4.0 branch. Is that OK with you, Richard?

Pinging early because this is a release blocker. :)

I don't see anything that looks amiss, but you should wait for @rsmith to approve.

lib/Sema/SemaChecking.cpp
2520

Thank you for this comment; I was about to ask about that very topic. :-D

11933

Unintended change?

lib/Sema/SemaOverload.cpp
6235

Braces might make this a bit easier to read due to the multiline if.

george.burgess.iv marked 2 inline comments as done.

Address feedback

Thanks for the feedback!

lib/Sema/SemaChecking.cpp
2520

ᕕ( ᐛ )ᕗ

Makes me kinda wish there was a clean way to say "the constness of the returned value depends on the constness of this arg," though. (For values of clean that don't involve some constness_of<ArgTy, RetTy>::type kind of thing)

rsmith edited edge metadata.Jan 27 2017, 2:08 PM

Another "fun" testcase:

struct S {
  void operator++(int n) _diagnose_if(n, "wat", "warning");
};
void f(S s) {
  s++; // no warning
  s.operator++(1); // warning
}
include/clang/Sema/Sema.h
2638–2639

Can you add a comment somewhere about here explaining why these functions are split? Something like "Argument-dependent diagnose_if attributes are checked when the function is used as a direct callee of a function call." here, and "Argument-independent diagnose_if attributes are checked on every use of the function." below.

9925

We have a loose convention that function parameter argument order matches source order, which would suggest that ThisArg should precede Args here.

lib/Sema/SemaExprCXX.cpp
6717

Can you call CheckFunctionCall instead here, and remove checkDiagnoseIfAttrsOnCall? It looks like the only reason we don't already call that is because none of its checks could ever fire for a call to a conversion function before, and that's no longer true.

lib/Sema/SemaOverload.cpp
12035

Likewise call CheckFunctionCall here.

12488

... and here.

13228

... and here.

test/SemaCXX/diagnose_if.cpp
614

constexpr is an adjective; "to a constant" might make more sense.

615

The range-based for is desugared to

auto &&__range = adl::Foo(1);
auto __begin = begin(__range);
auto __end = end(__range);
// ...

so the argument in the call to begin is not considered constant.

george.burgess.iv marked 7 inline comments as done.

Addressed all feedback

Another "fun" testcase

Ooh, shiny. Added.

test/SemaCXX/diagnose_if.cpp
615

Good to know. Thanks!

lib/Sema/SemaChecking.cpp
11933

...I dunno what keeps making this change, but I'll re-undo it before I submit.

rsmith accepted this revision.Jan 27 2017, 5:27 PM
This revision is now accepted and ready to land.Jan 27 2017, 5:27 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!