aaron.ballman added a reviewer: erichkeane.
aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:52-58
+ enum AttrPrintLoc {
+ SIDE_NONE = 0,
+ SIDE_LEFT = 1,
+ SIDE_MIDDLE = 2,
+ SIDE_RIGHT = 4,
+ SIDE_ANY = SIDE_LEFT | SIDE_MIDDLE | SIDE_RIGHT,
+ };
----------------
================
Comment at: clang/lib/AST/DeclPrinter.cpp:60-61
+
+ void prettyPrintAttributes(Decl *D, raw_ostream &out,
+ AttrPrintLoc loc = SIDE_ANY);
+
----------------
================
Comment at: clang/lib/AST/DeclPrinter.cpp:252-253
-void DeclPrinter::prettyPrintAttributes(Decl *D) {
+void DeclPrinter::prettyPrintAttributes(Decl *D, llvm::raw_ostream &out,
+ AttrPrintLoc loc) {
if (Policy.PolishForDeclaration)
----------------
================
Comment at: clang/lib/AST/DeclPrinter.cpp:267
+ // 3- should be print on the right side of a decl.
+ AttrPrintLoc attrloc = SIDE_MIDDLE; // We default to middle.
+ attr::Kind kind = A->getKind();
----------------
FWIW, I'm more used to seeing `__declspec` and `__attribute__` leading the
declaration specifiers instead of trailing them. Also, "middle" raises
questions for me as to where the attribute will go for a function. Given:
```
__attribute__((foo)) void bar(void);
```
what happens? All the test coverage that's changed currently is for variables,
not for functions, so I can't really tell.
Also, this should be tested with keyword attributes like `alignas` and
`__global` because those have very specific parse orders.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:273-274
+ if (A->isCXX11Attribute()) {
+ // C++11 onwards attributes can not be placed on middle. Output on
+ // right to mimic old clang behaviour.
+ attrloc = SIDE_RIGHT;
----------------
This comment isn't quite accurate -- it's more that `[[]]` attributes have very
specific rules about what they appertain to based on the syntactic location of
the `[[]]`. e.g.,
```
[[foo]] int a, b; // foo applies to both a and b
int [[foo]] a, b; // foo applies to the type of int, which forms the type for
both a and b
int a [[foo]], b; // foo applies to just a
```
Also, the same logic applies to `[[]]` in C as well as C++, so I think you
meant to use `A->isStandardAttributeSyntax()`.
================
Comment at: clang/test/Analysis/blocks.mm:73-74
+// FIXME: C++ issues a ignore warning on this __attribute__ output.
+
// CHECK-LABEL:void testBlockWithCaptureByReference()
----------------
Can you explain a bit more about what warning you're seeing and under what
condition?
================
Comment at: clang/test/Sema/attr-print.c:4
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
----------------
Why did you change where the attribute is written in this test?
================
Comment at: clang/test/Sema/attr-print.c:8-11
-__declspec(align(4)) int y;
+// CHECK: int __declspec(align(4)) y;
+int __declspec(align(4)) y;
-// CHECK: short arr[3] __attribute__((aligned));
-short arr[3] __attribute__((aligned));
----------------
Same questions here.
================
Comment at: clang/test/SemaCXX/attr-print.cpp:3-4
-// CHECK: int x __attribute__((aligned(4)));
-int x __attribute__((aligned(4)));
----------------
Same questions here as above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141714/new/
https://reviews.llvm.org/D141714
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
