Also introduce Expr::tryEvaluateStrLen.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
thanks for this! mostly just nits from me
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
15755 | Looks like this is the second "try to evaluate the call to this builtin function" API endpoint we have here (the other being for __builtin_object_size). IMO this isn't an issue, but if we need many more of these, it might be worth considering exposing a more general Expr::tryEvaluateBuiltinFunctionCall(APValue &, ASTContext &, BuiltinID, ArrayRef<Expr>) or similar. | |
clang/lib/Sema/SemaChecking.cpp | ||
604 | nit: i'd rename this ComputeExplicitObjectSizeArgument | |
621 | nit: would const Expr * work here? clang prefers to have const where possible | |
739 | i expected ComputeCheckVariantSize to imply that the argument was to a _chk function, but these cases don't reference _chk functions (nor do we set IsChkVariant = true;). should this be calling ComputeSizeArgument instead? | |
753 | same "shouldn't this be ComputeSizeArgument?' question | |
762 | same question | |
clang/test/Sema/warn-fortify-source.c | ||
64 | for completeness and consistency, please include a case where this warning doesn't fire. at the same time, it'd be nice to test for an off-by-one (which i believe is handled correctly by this patch already); maybe shorten src to "abcd" and have a test on char dst2[5]; that doesn't fire? |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
15737 | I just didn't find the extra verbiage helpful. AFAICT this code could be used for purposes other than issuing a diagnostic, and a diagnostic could be issued after following either the fast or slow path, so it didn't make a lot of sense to me. But if you have a different preference for this comment let me know. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
739 | Maybe the name ComputeCheckVariantSize was misleading and it'll be more clear now that I'm changing the name, but these functions like strncat, the memcpys below, and snprintf, etc, all take an explicit size argument just like the _chk functions. |
Rebased and addressed comments.
- Renamed ComputeCheckVariantSize.
- const Expr *.
- Changed existing test case to write only one excess byte.
- Added new, non-diagnosing test case.
lgtm -- thanks!
please give a day for other reviewers to add any last minute comments, then i think we can land this.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
739 | ohh, gotcha. i was misinterpreting the relationship between IsChkVariant and SourceSize then -- thanks for the clarification! |
Nit: s/null/NUL