[PATCH] D29599: Clang Changes for alloc_align

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+  let Spellings = [ GCC<"alloc_align"> ];
+  let Subjects = SubjectList<[Function]>;
----------------
Extra spaces in the declaration that do not match the style of the rest of the file (same happens below).


================
Comment at: include/clang/Basic/Attr.td:1225
+  let Spellings = [ GCC<"alloc_align"> ];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [ IntArgument<"ParamIndex"> ];
----------------
Is this subject line correct, or should it be using `HasFunctionProto` instead? How does GCC handle something like `void *blah() __attribute__((alloc_align(1)));` in C code?


================
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter, starting
+with 1.
----------------
I would split the "starting with 1" off into its own (full) sentence for clarity purposes. Also, please spell out one instead of 1.

You may also want to clarify how member functions do/do not impact this index.


================
Comment at: include/clang/Basic/AttrDocs.td:256
+.. code-block:: c++
+  // The returned pointer has the alignment specified by the first parameter
+  void *a(size_t align) __attribute__((alloc_align(1)));
----------------
I think you need a newline above this code block to not trigger sphinx diagnostics.


================
Comment at: lib/CodeGen/CGCall.cpp:4374
+    } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
+      llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
----------------
Instead of hoping all of the callers of `getParamIndex()` will remember that this is a weird one-based thing, why not give the semantic attribute the correct index when attaching the attribute to the declaration?


================
Comment at: lib/CodeGen/CGCall.cpp:4377
     }
+
   }
----------------
Spurious newline should be removed.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:222
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList
+template <class AttrInfo> static typename
----------------
Missing a full stop at the end of the comment.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:223
+/// AND the AttributeList
+template <class AttrInfo> static typename
+std::enable_if<std::is_base_of<clang::Attr, AttrInfo>::value, SourceLocation>::type
----------------
s/class/typename (same below).

Also, add some newlines between the function definitions and run the patch through clang-format (pointers and references are bound to the wrong thing and the formatting seems a bit off).


================
Comment at: lib/Sema/SemaDeclAttr.cpp:232
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList
+template <class AttrInfo> static typename
----------------
Missing a full stop at the end of the comment.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:775
 }
-
+//
 /// \brief Checks to be sure that the given parameter number is inbounds, and is
----------------
Should be removed.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:793
   const ParmVarDecl *Param = FD->getParamDecl(Idx);
+  if (AllowDependentType && Param->getType()->isDependentType()) {
+    return true;
----------------
Can elide the braces.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:805
 
+/// \brief Checks to be sure that the given parameter number is inbounds, and is
+/// an some integral type. Will emit appropriate diagnostics if this returns
----------------
s/inbounds/in bounds


================
Comment at: lib/Sema/SemaDeclAttr.cpp:806
+/// \brief Checks to be sure that the given parameter number is inbounds, and is
+/// an some integral type. Will emit appropriate diagnostics if this returns
+/// false.
----------------
Remove the "some".


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+  int IndexVal;
+  if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+                                /*Index=*/1))
----------------
It seems strange to me that you check that it's a positive integer argument before checking the param is an integer type.

Why not use `checkFunctionOrMethodParameterIndex()`?


================
Comment at: test/Sema/alloc-align-attr.c:5
+void test_void_alloc_align(void) __attribute__((alloc_align(1))); // expected-warning {{'alloc_align' attribute only applies to return values that are pointers}}
+int test_int_alloc_align(void) __attribute__((alloc_align(1))); // expected-warning {{'alloc_align' attribute only applies to return values that are pointers}}
+void *test_ptr_alloc_align(int a) __attribute__((alloc_align(1))); // no-warning
----------------
This test doesn't really add value.


================
Comment at: test/Sema/alloc-align-attr.c:11
+void *test_no_params() __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute parameter 1 is out of bounds}}
+void *test_insufficient_params(int a) __attribute__((alloc_align(2))); // expected-error {{'alloc_align' attribute parameter 2 is out of bounds}}
+void *test_incorrect_param_type(float a) __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
----------------
Same with this one.


================
Comment at: test/Sema/alloc-align-attr.c:16
+void *test_bad_param_type(void) __attribute((alloc_align(1.1))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
+void *test_bad_param_type(void) __attribute((alloc_align("Foo"))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
+
----------------
Same.


================
Comment at: test/SemaCXX/alloc-align-attr.cpp:11
+  T Foo2(int a) __attribute__((alloc_align(2)));// expected-warning {{'alloc_align' attribute only applies to return values that are pointers or references}}
+
+};
----------------
Spurious newline.


https://reviews.llvm.org/D29599