erik.pilkington marked 8 inline comments as done. erik.pilkington added inline comments.
================ Comment at: clang/include/clang/Basic/AttrDocs.td:987-989 +The first argument to the attribute is the type passed to +`__builtin_object_size`, and the second is the flag that the fortified format +functions accept. ---------------- aaron.ballman wrote: > erik.pilkington wrote: > > aaron.ballman wrote: > > > Maybe I'm being dense, but I still have no idea what I'd pass for either > > > of these arguments. When I hear "type", I immediately assume I should > > > pass in something like 'int', but that seems wrong given that this is an > > > integer argument. Is there some public documentation we could link to > > > with a "for more information, see <blah>" snippet? Similar for the > > > fortified format function flag. > > > > > > However, I'm also starting to wonder why this attribute is interesting to > > > users. Why not infer this attribute automatically, since there's only a > > > specified set of standard library functions that it can be applied to > > > anyway? Is it a bad idea to try to infer this for some reason? > > Yeah, I guess we could link to GCC's docs for `__builtin_object_size`, I'll > > update this. I think the flag argument has something to do with enabling > > checking `%n` output parameters, but I'm not totally sure and don't want to > > spread any misinformation in the clang docs. On our implementation, the > > value is just ignored. > > > > This attribute would never really be used by users that aren't C library > > implementers. The problem with doing this automatically is that library > > users need to be able to customize the checking mode by defining the > > `_FORTIFY_SOURCE` macro to a level in their `.c` files. We can't do this > > based on the presence of that macro for a couple reasons, firstly, I'm not > > sure we can assume that all `*_chk` variants are present just because > > `_FORTIFY_SOURCE` is defined (whether the `_chk` variants are present seems > > like a decision best left to the library). And secondly because that would > > mean that `clang -E t.c -o - | clang -xc -` would generate different code > > from `clang t.c`. Given that, it seems like an attribute is the best > > customization point. > Thank you for the explanation -- I agree that an attribute probably makes > sense then. I'd appreciate a note in the docs saying something along the > lines of "This attribute is intended for use within standard C library > implementations and should not generally be used for user applications." or > some such. WDYT? Sure, I added that in the commit. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6437 + S.Diag(AL.getArgAsExpr(0)->getBeginLoc(), + diag::err_attribute_argument_outof_range) + << AL << 0 << 3; ---------------- aaron.ballman wrote: > lol, I'll fix that typo after you land your patch. Thanks, guess you're less lazy then me :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57918/new/ https://reviews.llvm.org/D57918 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
