aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:5973
+``mymemset(n, c, s)`` will diagnose overflows as if it were the call
+``__builtin_memset(s, c, n)``;
+}];
----------------
One thing that's not quite clear from the docs is how to handle variadic arg
builtins like `__builtin_printf` -- how do you specify "the variadic args go
here"?
Another thing that's unclear is whether you can apply this attribute to a
member function, and if so, how does that change the indexes given for the
implicit `this` argument?
We should add test coverage for both of these situations.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:673
- auto ComputeExplicitObjectSizeArgument =
+ const auto TranslateIndex = [&](unsigned Index) -> Optional<unsigned> {
+ if (UseDAIAttr) {
----------------
We typically don't use top-level `const` on value types for local
variables/parameters (only on pointers or references). (same comment applies
elsewhere)
================
Comment at: clang/lib/Sema/SemaChecking.cpp:692
return llvm::None;
- return Result.Val.getInt();
+ auto Integer = Result.Val.getInt();
+ Integer.setIsUnsigned(true);
----------------
Please spell out the type.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1006
+ const ParsedAttr &AL) {
+ const auto *DeclFD = dyn_cast_or_null<FunctionDecl>(D);
+ if (!DeclFD)
----------------
I don't believe `D` can ever be null.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1014-1018
+ if (Union.is<Expr *>()) {
+ return Union.get<Expr *>()->getBeginLoc();
+ } else {
+ return Union.get<IdentifierLoc *>()->Loc;
+ }
----------------
Removes curly braces and an `else` after a `return`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1040
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments_for)
+ << AL << AttrFD->getName() << AttrFD->getNumParams();
+ return;
----------------
Passing in a `NamedDecl` will be automatically quoted by the diagnostics
engine, while passing in a `StringRef` will not. We prefer syntax to be quoted
in diagnostics to reduce confusion.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1055-1057
+ if (!checkUInt32Argument(S, AL, IndexExpr, Index, I + 1, false)) {
+ return;
+ }
----------------
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1061
+ S.Diag(AL.getLoc(), diag::err_attribute_bounds_for_function)
+ << AL << Index << DeclFD->getName() << DeclFD->getNumParams();
+ return;
----------------
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1065-1066
+
+ const auto T1 = AttrFD->getParamDecl(I - 1)->getType();
+ const auto T2 = DeclFD->getParamDecl(Index - 1)->getType();
+
----------------
Please spell out the type as it's not spelled out in the initialization.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1068-1069
+
+ if (T1.getCanonicalType().getUnqualifiedType() !=
+ T2.getCanonicalType().getUnqualifiedType()) {
+ S.Diag(IndexExpr->getBeginLoc(), diag::err_attribute_parameter_types)
----------------
This may be fine, but it may also be overly restrictive. Consider:
```
void f(void *dest, char val, size_t n)
__attribute__((diagnose_as_builtin(__builtin_memset, 1, 2, 3))) {}
```
I think this will get diagnosed because of the type mismatch between `char` and
`int`, won't it? But notionally, the argument passed to `val` will be promoted
to `int` anyway, so they're morally equivalent. This would be a good test case
to add.
FWIW, I prefer erring on the side of being overly restrictive (we can relax
restrictions more easily than we can add new ones), which is why I think this
is fine for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112024/new/
https://reviews.llvm.org/D112024
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
