Page MenuHomePhabricator

[Consumed] Refactor and improve diagnostics
AcceptedPublic

Authored by comex on Sep 18 2019, 4:24 PM.

Details

Reviewers
dblaikie
Summary

As suggested by FIXME comments, fix commented-out diagnostic in Sema and remove the equivalent check within the consumed analysis.

The diagnostic in question is the warning for using param_typestate and return_typestate with a type that is not consumable.

There were several FIXME comments about the same issue; the most detailed was before the commented-out check:

// FIXME: This check is currently being done in the analysis.  It can be
//        enabled here only after the parser propagates attributes at
//        template specialization definition, not declaration.

I was confused what this meant. After investigating, I think it actually refers to the fact that attributes are parsed only once for a template, and are not re-parsed when the template is instantiated. If I'm right, the issue actually has nothing to do with template specializations or with definitions versus declarations. I could be missing something, though, so please let me know if there's a case I'm not thinking of. I did add some template specializations as test cases (for both class and function templates).

This patch addresses the issue by moving the diagnostic to a function which is called from both parsing and template instantiation, similar to what's already done with some other attributes.

The analysis version of the check didn't always work, and only applied to return_typestate rather than param_typestate (even though both had commented-out Sema checks); therefore, the fixed code may produce warnings that didn't appear before.

Also, add a new diagnostic for when the set_typestate or test_typestate attribute is applied to a constructor or static method; previously Clang would ignore it or crash, respectively. (removed)

Diff Detail

Repository
rC Clang

Event Timeline

comex created this revision.Sep 18 2019, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 4:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
comex updated this revision to Diff 220784.Sep 18 2019, 6:17 PM

Fixed a mistake where the diagnostic would print the wrong parameter type.

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

include/clang/Basic/DiagnosticSemaKinds.td
3246

"member function" would be more correct than "method" here (the diagnostics in this file using the word "method" are mostly for Objective C things)

Are there other diagnostics that are similar? (like maybe function "const" - which can't be on non-member, static member, or ctors - wouldn't that be the same here? Where's the failure path for a non-member (free) function? Could it be unified with the static member/ctor case you're adding?)

Ah, looks like in Attr.td these attributes could be flagged "NonStaticCXXMethod" rather than "CXXMethod" - perhaps that narrower classification wasn't available when this was originally implemented. (then, I think, the attribute parsing logic will do the job of warning about the misuse and ignoring the attribute entirely)

lib/Sema/SemaDeclAttr.cpp
1229–1232
comex marked 4 inline comments as done.Sep 19 2019, 4:30 PM

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

Okay, I'll take the set_typestate/test_typestate part out of this and submit it separately.

include/clang/Basic/DiagnosticSemaKinds.td
3246

That doesn't check for constructors, but sure, I'll add a "NonStaticNonConstructorCXXMethod" instead (in the separate submission).

lib/Sema/SemaDeclAttr.cpp
1229–1232

Done.

comex updated this revision to Diff 220918.Sep 19 2019, 4:32 PM
comex marked 2 inline comments as done.
comex edited the summary of this revision. (Show Details)

Addressed feedback.

dblaikie accepted this revision.Thu, Sep 26, 5:57 PM
dblaikie added inline comments.
lib/Sema/SemaDeclAttr.cpp
1249

Do you need the "? 0 : 1" here? Or would the boolean value be sufficient to use directly?

lib/Sema/SemaTemplateInstantiateDecl.cpp
609–610

Collapse these two conditions into one 'if', perhaps?

This revision is now accepted and ready to land.Thu, Sep 26, 5:57 PM