rjmccall added inline comments.
================
Comment at: lib/Parse/ParseDecl.cpp:6162
+ }
+ }
+
----------------
Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > This is enforcing a restriction that users write `const __private`,
> > > > which seems unreasonable. It looks like `ParseTypeQualifierList` takes
> > > > a flag saying not to parse attributes; try adding a new option to that
> > > > enum allowing address-space attributes.
> > > >
> > > > Collecting the attributes on the type-qualifiers `DeclSpec` rather than
> > > > adding them as function attributes seems correct.
> > > Do you mean `ParseTypeQualifierListOpt`? That already parses the address
> > > spaces unconditionally. The problem is however that we are using local
> > > `DeclSpec` - `DS` variable here during the function qualifiers parsing
> > > because the `DeclSpec` member of the `Declarator` corresponds to the
> > > return type as far as I understand it. Therefore I am propagating missing
> > > address space attributes from the local `DS` variable into `FnAttrs` to
> > > be used in the function type info.
> > >
> > > With current patch both of the following two forms:
> > > struct C {
> > > void foo() __local const;
> > > };
> > > and
> > > struct C {
> > > void foo() const __local;
> > > };
> > > would be parsed correctly generating identical IR
> > > declare void @_ZNU3AS3K1C3fooEv(%struct.C addrspace(3)*)
> > >
> > >
> > >
> > Oh, I see, sorry. Why filter on attributes at all, then? We should *only*
> > be parsing qualifier attributes in that list.
> >
> > I actually think it would be reasonable to change
> > `DeclaratorChunk::FunctionTypeInfo` to just store a `DeclSpec` for all the
> > qualifiers; we're already duplicating an unfortunate amount of the logic
> > from `DeclSpec`, like remembering `SourceLocation`s for all the qualifiers.
> > You'll have to store it out-of-line, but we already store e.g. parameters
> > out of line, so the machinery for allocating and destroying it already
> > exists. Just make sure we don't actually allocate anything in the common
> > case where there aren't any qualifiers (i.e. for C and non-member C++
> > functions).
> >
> > Also, I suspect that the use of `CXXThisScopeRAII` below this needs to be
> > updated to pull *all* the qualifiers out of `DS`. Maybe Sema should have a
> > function for that.
> I have uploaded a separate patch for this:
> https://reviews.llvm.org/D55948
>
> I think I can't avoid storing `DeclSpec` for non-memeber functions in C++
> because we still use them to give diagnostics about the qualifiers. For
> example:
> test.cpp:1:12: error: non-member function cannot have 'const' qualifier
> void foo() const;
>
> As for `CXXThisScopeRAII`, we seem to be adding other qualifiers to the
> `QualType` directly (while building it), so there seems to be no function to
> collect them into `Qualifiers`. I can, however, add code to collect address
> space qualifiers here. I guess we don't have any other missing qualifiers to
> collect?
>
I think you can probably avoid allocating and storing a `DeclSpec` if there are
no qualifiers, which should be easy to test, and then the absence of a
`DeclSpec` should allow Sema to quickly bail out of the check for illegal
qualifiers on a non-member function type.
> I guess we don't have any other missing qualifiers to collect?
Not that are legal to apply to `this`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55850/new/
https://reviews.llvm.org/D55850
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
