[clang-tidy] Prohibit the use of the C standard library for character classification
Categories
(Developer Infrastructure :: Source Code Analysis, enhancement, P3)
Tracking
(Not tracked)
People
(Reporter: hsivonen, Assigned: freddy)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
The C standard library has functions for classifying characters, such as isalpha() and isdigit(). Using these is always wrong. They are wrong to use when working with natural-language text, because they only deal with values in the range of char or the EOF constant instead of dealing with the full Unicode range. They are wrong when working with computer syntaxes, because they are locale-dependent. Even the ones that aren't locale-dependent according to the C standard are locale-dependent on Windows. https://stackoverflow.com/questions/2898228/can-isdigit-legitimately-be-locale-dependent-in-c I suggest adding an analysis that complains about the use of these functions (or their C++ analogs under the std:: namespace) and suggests the use of mbft/TextUtils.h instead.
Comment 1•6 years ago
|
||
This is very easy to implement, it can be done very easy at the matcher level, an example can be found here: https://bugzilla.mozilla.org/show_bug.cgi?id=1391711.
Reporter | ||
Comment 2•6 years ago
|
||
Also strcasecmp and strncasecmp from strings.h are locale-dependent.
Comment 4•6 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=1452693#c1
Comment 5•6 years ago
|
||
not fixed, sorry
Updated•4 years ago
|
Comment 7•4 years ago
|
||
Work in progress. I don't know how to add exceptions for existing code. Is
there an existing lint that has something like that?
Comment 8•4 years ago
|
||
There are several copies of gtest in our tree, all listed in tools/rewriting/ThirdPartyPaths.txt
. Nevertheless, they are triggering these warnings as though they were first-party code, because we link them into the OBJDIR:
$OBJDIR/dist/include/gtest/internal/gtest-port.h -> $SRCDIR/testing/gtest/gtest/include/gtest/internal/gtest-port.h
Is this a known issue?
Comment 9•4 years ago
|
||
Yes, please see this workaround https://searchfox.org/mozilla-central/source/build/clang-plugin/CustomMatchers.h#137
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Depends on D81785
Comment 11•4 years ago
|
||
Pushed by jorendorff@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fad2fb1e60ee Part 1: Clang-plugin checker for locale-specific functions. r=hsivonen. https://hg.mozilla.org/integration/autoland/rev/3f6600510057 Part 2: Suppress warnings in a gtest file. r=andi
Comment 12•4 years ago
|
||
Backed out for bustages on TestNoLocaleSpecificChecker.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/2f02cadc189cd3b46183ddcacad2171a8e3fef93
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310374949&repo=autoland&lineNumber=8431
Comment 13•4 years ago
|
||
Hmm. strcasecmp
doesn't exist on Windows. By far the easiest thing is to delete that particular part of the test, so I guess I'll do that.
Comment 14•4 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #13)
Hmm.
strcasecmp
doesn't exist on Windows. By far the easiest thing is to delete that particular part of the test, so I guess I'll do that.
Does that mean we should be checking for the Windows equivalents as well?
Reporter | ||
Comment 15•4 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #14)
Does that mean we should be checking for the Windows equivalents as well?
It indeed appears that we should block _stricmp
, _wcsicmp
, and _mbsicmp
, too.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Description
•