[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8175
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
-  if (FTI.TypeQuals != 0) {
-    if (FTI.TypeQuals & Qualifiers::Const)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "const" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Volatile)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "volatile" << SourceRange(D.getIdentifierLoc());
-    if (FTI.TypeQuals & Qualifiers::Restrict)
-      Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-        << "restrict" << SourceRange(D.getIdentifierLoc());
+  if (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers() != 0) {
+    auto DiagQual = [&](DeclSpec::TQ TypeQual, StringRef QualName,
----------------
rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > 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.
> > > > 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.
> > > Adding another method might be easier.  How many clients actually use the TQ?
> > 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?
> 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.
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. 


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

https://reviews.llvm.org/D55948