Page MenuHomePhabricator

[analyzer] Limit UCharMax to min of max uchar or max int
ClosedPublic

Authored by vabridgers on Mar 3 2020, 8:04 AM.

Details

Summary

This change is a follow up to commit 536456a7e93d73b9ff4e92f3e51d1aa1c72628fe
and limits UCharMax to the min of the maximum values for the
architecture's maximum unsigned char or maximum int values. This is
required for architectures where these values are different than the
most commonly supported architectures.

Diff Detail

Event Timeline

vabridgers created this revision.Mar 3 2020, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 8:04 AM

Could you add a test please? We really need tests for every patch.

martong accepted this revision.Mar 3 2020, 9:03 AM

Looks good to me! Thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
514

Perhaps it would be more precise to refer to the argument of these functions. So The C standard states that the arguments of functions like isalpha must be representable ...

517

Could you please spell out ISA? Maybe it is just me but I don't know what that stands for.

This revision is now accepted and ready to land.Mar 3 2020, 9:03 AM
vabridgers updated this revision to Diff 247936.Mar 3 2020, 9:32 AM

Address review comments

vabridgers marked 2 inline comments as done.Mar 3 2020, 9:34 AM
vabridgers added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
514

Will do.

517

Yes, will do.

@Charusso, I cannot find a way to create a test for this change, since this was found using an architecture that supports 16-bit chars. I understand the need to create test cases for changes. In this case, I believe a rationale for accepting this change is to conform to the C Standard, and accept that upstream clang does not have a way to test for architectures supporting 16-bit characters, or otherwise "peculiar" datatypes.

Please do indicate if you're willing to accept this patch or not. If not, I need to know so I solve this problem for my customer in a different way.

Thank you - Vince

Charusso accepted this revision.Mar 9 2020, 10:29 AM

@Charusso, I cannot find a way to create a test for this change, since this was found using an architecture that supports 16-bit chars.

I really felt that we could write a test that using a target which behaves like that. I am not that awesome with triples, sorry. If I am right we are good to go without @NoQ's response (he is the maintainer of the Analyzer). Thanks!

NoQ added a comment.Mar 10 2020, 12:20 AM

Aha, ok, i appreciate making life easier for downstream users.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
519

Let's rename this constant then, so that we still had our UCharMax when we actually need the real UCHAR_MAX in the summary.

Szelethus accepted this revision.Mar 10 2020, 4:13 AM

I have nothing else to add :)

Address Artem's comments.

vabridgers marked 2 inline comments as not done.

fix pre merge checkswq

vabridgers marked 6 inline comments as done.Mar 12 2020, 5:40 PM

I believe all comments have been addressed. Please let me know if there's anything else required. Thanks

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
519

Done, thanks Artem.

I believe all comments have been addressed. Please let me know if there's anything else required. Thanks

I think you have solved everything. Do you have commit access? May you would request it: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

vabridgers marked an inline comment as done.Mar 13 2020, 10:30 AM

@Charusso -- I do not have commit access, but I will request. Thank you.

NoQ added a comment.Mar 15 2020, 9:18 PM

@Charusso -- I do not have commit access, but I will request. Thank you.

Whoops, i accidentally landed this patch because i misread your statement as if you're asking to land the patch for you :) Please request commit access anyway :)

This revision was automatically updated to reflect the committed changes.