leonardchan added inline comments.
================ Comment at: lib/AST/TypePrinter.cpp:1370 + + // Remove the underlying address space so it won't be printed. + SplitQualType SplitTy = T->getModifiedType().split(); ---------------- rsmith wrote: > leonardchan wrote: > > rsmith wrote: > > > leonardchan wrote: > > > > rsmith wrote: > > > > > This is unnecessary; just print the modified type here. (The modified > > > > > type by definition does not have the attribute applied to it.) > > > > When you say the modified type, do you mean just the type without it's > > > > qualifiers? I wasn't sure if removing all the qualifiers would suffice > > > > since there were also other non-address_space qualifiers that could be > > > > printed. > > > I mean `T->getModifiedType()`, which tracks what the type was before the > > > attribute was applied. > > Oh, I understand that you meant `T->getModifiedType()`. This is a special > > case when printing the `address_space` since even though the attribute is > > applied and printed here, when we reach the qualifiers of the modified > > type, the address space will still be printed unless we remove it here. > > > > I'm not sure if there's a better way to do this though. > If the address space qualifiers are present on the modified type, then that's > a bug in the creation of the `AttributedType`. And indeed looking at r340765, > I see we are passing the wrong type in as the modified type when creating the > `AttributedType`. Please fix that, and then just use `getModifiedType` here. Making another patch for this. ================ Comment at: lib/Lex/PPDirectives.cpp:2584 +// one token for the attribute itself. +static constexpr unsigned kMinAttrTokens = 6; + ---------------- aaron.ballman wrote: > This count is specific to GNU spellings. For instance, a C++ spelling might > have 5 tokens (two square brackets, attribute-token, two more square > brackets) or more and a declspec spelling might have 4 tokens (__declspec > keyword, paren, attribute token, paren). See comment below ================ Comment at: lib/Lex/PPDirectives.cpp:2586-2588 +/// This only catches macros whose whole definition is an attribute. That is, it +/// starts with the attribute keyword and 2 opening parentheses, and ends with +/// the 2 closing parentheses. ---------------- aaron.ballman wrote: > Why does this need to be specific to GNU-style spellings? Similar to the `noderef` patch I sent out, this is initially only meant for GNU-style attributes but can be expanded on later. The main goal for our side was to change the diagnostics relating to address_spaces to instead print the macro if the attribute was defined in a macro. This was initially discussed in http://lists.llvm.org/pipermail/cfe-dev/2018-August/058702.html, but eventually decided on not using a pragma and instead redirecting diagnostics (http://lists.llvm.org/pipermail/cfe-dev/2018-August/058732.html). Essentially, this was meant specifically for just the `address_space` attribute, which I believe only has GNU-style support, but Richard Smith thought that this would be useful to apply to other attributes. For our use case alone, we just need it to work for GNU-style, but we can expand on this afterwards for other attribute spellings. ================ Comment at: lib/Lex/PPDirectives.cpp:2720-2721 + // If the macro is an attribute that contains address_space(), save this for + // diagnosing later. + SourceRange Range; ---------------- aaron.ballman wrote: > This should not be specific to `address_space`, but even if it were, this is > wrong as it can be spelled `[[clang::address_space(0)]]`. Removed code relating to the processor since we don't need it anymore ================ Comment at: lib/Parse/ParseDecl.cpp:116 +/// they were defined in. +static void ApplyMacroIIToParsedAttrs(ParsedAttributes &attrs, unsigned N, + IdentifierInfo *MacroII) { ---------------- aaron.ballman wrote: > `attrs` doesn't meet the usual naming conventions. Same with `i` below. Removed ================ Comment at: lib/Parse/ParseDecl.cpp:119 + assert(attrs.size() >= N); + for (unsigned i = attrs.size() - N; i < N; ++i) + attrs[i].setMacroII(MacroII); ---------------- aaron.ballman wrote: > You can use a range-based for loop here instead. The condition should instead be `i < attrs.size()`, sorry. I don't know if `ParsedAttributesView` has a reverse iterator though or I can start with an iterator in the middle of the view. ================ Comment at: lib/Parse/ParseDecl.cpp:246-252 + for (const auto &MacroPair : PP.getAttributeMacros()) { + if (SourceLocInSourceRange(AttrTok.getLocation(), MacroPair.first, + PP.getSourceManager())) { + ApplyMacroIIToParsedAttrs(attrs, NumParsedAttrs, MacroPair.second); + break; + } + } ---------------- rsmith wrote: > From my earlier comment: "look through the source location information for > the outermost macro expansion that exactly covers the token sequence in > question, and that's your corresponding macro name. (Note that we don't need > to check what the expansion of the macro is, nor track what attribute-like > macros have been defined, to support this approach.)" > > We should not be tracking a list of attribute-like macros in the > preprocessor. Doing so is unnecessary and problematic for various reasons. Removed the preprocessor code Repository: rC Clang https://reviews.llvm.org/D51329 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
