Page MenuHomePhabricator

Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers
ClosedPublic

Authored by Anastasia on Dec 20 2018, 12:31 PM.

Details

Summary

Rather than duplicating data fields, use DeclSpec directly to store qualifiers for the functions/methods.

This change is discussed in the following comment:
https://reviews.llvm.org/D55850#inline-495037

Diff Detail

Repository
rC Clang

Event Timeline

Anastasia created this revision.Dec 20 2018, 12:31 PM
rjmccall added inline comments.Dec 20 2018, 1:40 PM
include/clang/Sema/DeclSpec.h
1316

Can we give this a better name? I know we use "type qualifiers" for this concept in the AST, but it's not a good name, and we should really rename it in the AST. How about MethodQualifiers?

lib/Parse/ParseDecl.cpp
6126

Is it possible to just build into a local DS and then move that DS to the heap if it contains qualifiers or attributes? It'd be nice to not do a heap allocation every time we parse a function declarator in C++ since the vast majority of them don't have qualifiers. (It probably has to be a move to handle attributes and AttributeFactory correctly.)

That would also side-step any concerns about memory management.

Anastasia updated this revision to Diff 179469.Dec 24 2018, 6:24 AM
  • Changed to store DeclSpec only when quals are present.
  • Renamed to MethodQualifiers.
Anastasia marked 3 inline comments as done.Dec 24 2018, 6:30 AM
Anastasia added inline comments.
lib/Parse/ParseDecl.cpp
6126

I found DeclSpec a bit copy-unfriednly... not sure it was better to modify its interface. For now I just provide custom impl for function quals.

rjmccall added inline comments.Dec 31 2018, 12:41 PM
include/clang/Sema/DeclSpec.h
1381

This DeclSpec ends up with a dangling reference to the AttributeFactory; I think you need to allocate them both when used.

Consider having this return a DeclSpec & and making it the primary way that clients get a mutable DS.

1422–1423

The "if any" suggests that that this method (and below) can be called on a DS that doesn't have this qualifier. If that's not actually true, you should assert that MethodQualifiers exists and change the comment.

1462

It might be reasonable to just remove these accessors and make more things query the DS if it exists.

1611

Spacing

lib/Parse/ParseDeclCXX.cpp
2351–2352

Since you're touching this code anyway, please make this an early return (before the declaration of Insertion) and de-indent the rest of the helper.

2352

Either MethodQualifiers already exists (because it has the qualifier) or you'll be creating it (to add the qualifier); might as well force it to exist eagerly and simplify the logic here and below.

2363–2364

Please remove this dead semicolon since you're touching this code anyway.

lib/Sema/DeclSpec.cpp
175

Consider taking this as a pointer just so that the various places that construct one of these when method qualifiers are impossible don't have to construct a DS unnecessarily.

lib/Sema/SemaDeclCXX.cpp
8363–8368

Should there be a method on DS that iterates over the type qualifiers present? You could have it take an llvm::function_ref<void(TQ, StringRef, SourceLoc)> or something.

Anastasia updated this revision to Diff 179877.Jan 2 2019, 9:51 AM
Anastasia marked an inline comment as done.

Addressed review comments.

Anastasia marked an inline comment as done.Jan 2 2019, 9:52 AM
Anastasia added inline comments.
lib/Parse/ParseDeclCXX.cpp
2363–2364

Here the semicolon is at the end of a lambda declaration statement.

rjmccall added inline comments.Jan 2 2019, 10:26 AM
lib/Parse/ParseDeclCXX.cpp
2363–2364

Oh, yes it is, sorry.

lib/Parse/ParseExprCXX.cpp
1264

This is dead now.

lib/Sema/DeclSpec.cpp
233

Spacing?

lib/Sema/SemaDeclCXX.cpp
8177–8182

I think you should add a hasMethodQualifiers method to FTI that does this check. Note that it needs to check for attributes, too, and I think you need to figure out some way to generalize forEachCVRUQual to cover those.

8183

I'd write this lambda inline as the argument, but if you think it's better separate, that's okay.

8363–8368

Another place for hasMethodQualifiers.

lib/Sema/SemaType.cpp
719

This is dead now.

4459

This would be a good place to use hasMethodQualifiers (unless there's a reason to ignore attribute qualifiers here).

5033

A lambda this simple should definitely be written inline.

Anastasia marked 2 inline comments as done.Jan 2 2019, 10:51 AM
Anastasia added inline comments.
lib/Sema/DeclSpec.cpp
233

I am keeping the old formatting from the lines 195-204. But obviously it doesn't match with the current clang-format rules. Do you prefer that I reformat the whole block or just this line?

lib/Sema/SemaDeclCXX.cpp
8177–8182

Are there any attributes I should handle currently?

Also are you suggesting to add another forEach... method or extend existing? If the latter, I might not be able to use it in all places I use it now.

rjmccall added inline comments.Jan 2 2019, 11:18 AM
lib/Sema/DeclSpec.cpp
233

Oh, I see. Could you hoist the null-initialization of these two fields above this block, then? They'll look more consistent, and it'll make it clearer that you're establishing the basic invariants on these fields before modifying them.

lib/Sema/SemaDeclCXX.cpp
8177–8182

Adding another method might be easier. How many clients actually use the TQ?

Anastasia marked an inline comment as done.Jan 3 2019, 2:48 AM
Anastasia added inline comments.
lib/Sema/SemaDeclCXX.cpp
8177–8182

In DeclSpec.cpp I definitely need just TQ. I am not sure about SemaType.cpp. All other places (3x) I guess should be possible to generalize. Although I am not very clear if I should be checking all attr. It might be a bit exhaustive since the use cases are for the function?

Perhaps, I could add an extra helper forEachQualifier that can call forEachCVRUQual and then I could add a FIXME to complete the rest. We can extend it as we discover what's missing. For example I will add address spaces there in my next patch. Would this make sense?

As for hasMethodQualifiers just to be clear I would need to check for all qualifiers including reference qualifier, attributes, etc?

rjmccall added inline comments.Jan 3 2019, 8:38 AM
lib/Sema/SemaDeclCXX.cpp
8177–8182

That seems like a reasonable short-term plan. Maybe there needs to be some way to describe an individual qualifier; we can hash that out in a separate patch.

As for hasMethodQualifiers just to be clear I would need to check for all qualifiers including reference qualifier, attributes, etc?

Maybe, although at least one of the cases below wants to check for ref-qualifiers separately. Maybe it should be hasMethodTypeQualifiers, and it implies that MethodQualifiers->forEachQualifier will invoke the callback at least once.

Anastasia updated this revision to Diff 180226.Jan 4 2019, 4:57 AM
Anastasia marked an inline comment as done.
  • Added forEachQualifiers to DeclSpec.
  • Removed dead code.
  • Moved lambdas definition inline into the function parameters.
lib/Sema/SemaDeclCXX.cpp
8177–8182

I think it should be sufficient to check that MethodQualifiers exist because we only create it if we have either a type qual or any attribute.

rjmccall added inline comments.Jan 4 2019, 8:38 AM
lib/Sema/SemaDeclCXX.cpp
8177–8182

Hmm. I think we can remove qualifiers, and we do sometimes (at least in recovery situations, and possibly in others), so it's better not to rely on that. But I think for attributes you can just check whether there are any attributes rather than carefully checking whether any attribute is a type qualifier.

Anastasia updated this revision to Diff 180464.Jan 7 2019, 5:00 AM

Added hasMethodTypeQualifiers method.

rjmccall accepted this revision.Jan 7 2019, 12:28 PM

Thanks! LGTM.

This revision is now accepted and ready to land.Jan 7 2019, 12:28 PM
This revision was automatically updated to reflect the committed changes.