kulpreet marked 14 inline comments as done.
kulpreet added a comment.
Thanks for the feedback! Uploading a new diff with the changes.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:13
@@ +12,3 @@
+// UI methods expecting localized strings
+// 2) A syntactic checker that warns against not including a comment in
+// NSLocalizedString macros.
----------------
zaks.anna wrote:
> Is the comment argument optional in some cases or is it always encouraged in
> some contexts?
Engineers sometimes assume that a particular string alone will have enough
context for the translators to figure out the meaning, but this usually a bad
assumption. Similar to the way that it is "bad practice" to not have
descriptive variable and function names (i.e. foo()), it's "bad practice" to
not include a comment in a localized string. I modified my comment to clarify.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:39
@@ +38,3 @@
+private:
+ enum Kind { Unknown, NonLocalized, Localized } K;
+ LocalizedState(Kind InK) : K(InK) {}
----------------
zaks.anna wrote:
> Unknown is never used?
Removing Unknown from the enum.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:66
@@ +65,3 @@
+ mutable std::map<StringRef, std::map<StringRef, uint8_t>> UIMethods;
+ mutable llvm::SmallSet<std::pair<StringRef, StringRef>, 10> LSM;
+ mutable llvm::StringMap<char> LSF; // StringSet doesn't work
----------------
zaks.anna wrote:
> Please, use more descriptive names here (or add a comment on what these are).
> Why StringSet does not work?
Adding in comments.
When I try to use StringSet I get:
`error: no type named 'StringSet' in namespace 'llvm';`
even when I explicitly #include "llvm/ADT/StringSet.h"
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:201
@@ +200,3 @@
+
+ ExplodedNode *ErrNode = C.addTransition();
+ if (!ErrNode)
----------------
zaks.anna wrote:
> I believe this would just return a predecessor and not add a new transition.
> If you want to create a new transition, you should tag the node with your
> checker info. For example, see CheckerProgramPointTag in the MallocChecker.
I'm not 100% clear on why I would want to create a new transition, but I
adjusted to code so it does something similar to MallocChecker. The alternative
is to just call getPredecessor() which seems to work the same?
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:208
@@ +207,3 @@
+ new BugReport(*BT, "String should be localized", ErrNode));
+ R->addRange(M.getSourceRange());
+ R->markInteresting(S);
----------------
zaks.anna wrote:
> You can try reporting a more specific range here, for example the range of
> the argument expression if available. This is what gets highlighted in Xcode.
Good idea. Adding this for arguments so it highlights the string.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:235
@@ +234,3 @@
+ // These special NSString methods draw to the screen
+ StringRef drawAtPoint("drawAtPoint");
+ StringRef drawInRect("drawInRect");
----------------
zaks.anna wrote:
> Are these temporaries needed?
No, these will be removed.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:322
@@ +321,3 @@
+ } else {
+ // Perhaps I can use a different heuristic for non-aggressive reporting?
+ const SymbolicRegion *SymReg =
----------------
zaks.anna wrote:
> Could you add a comment documenting what it is?
> Is it that the literals other than the empty string are assumed to be
> non-localized?
>
Adding a comment to the method signature, clarifying this.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:333
@@ +332,3 @@
+/// if it is in LocStringMethods or the method is annotated
+void NonLocalizedStringChecker::checkPostObjCMessage(const ObjCMethodCall &msg,
+ CheckerContext &C) const {
----------------
zaks.anna wrote:
> Why are we not using the same heuristic as in the case with other calls?
I think we are? checkPostCall should also cover ObjC methods for the heuristic.
This function is to specifically cover the case where we need to mark a value
returned by a method in LocStringMethods (LSM) as localized.
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:365
@@ +364,3 @@
+ StringRef stringValue = SL->getString()->getString();
+ SVal sv = C.getSVal(SL);
+ if (stringValue.empty()) {
----------------
zaks.anna wrote:
> Is it possible to see that we are dealing with an empty string from an SVal?
> That way you could keep the state lean.
Good idea. I was able to do this with the following:
```
if (const ObjCStringRegion *SR =
dyn_cast_or_null<ObjCStringRegion>(svTitle.getAsRegion())) {
StringRef stringValue =
SR->getObjCStringLiteral()->getString()->getString();
if (stringValue.empty())
return;
}
```
And then marking all string literals here as NonLocalized
================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:450
@@ +449,3 @@
+ // an NSLocalizedString macro
+ SourceLocation SL =
+ Mgr.getSourceManager().getImmediateMacroCallerLoc(R.getBegin());
----------------
zaks.anna wrote:
> Would this be susceptible to the macro definition changes?
>
> What the while loop below is doing?
>
> I don't think this will work if the expansion is inside of another macro
> expansion, so we should check for that case and not warn if that is the case.
This should work for any macro for [NSBundle
localizedStringForKey:Value:Table:] where the last argument holds the comment.
I have a test in localization-aggressive.m that checks for the macro expansion
within a macro expansion and it seems to be working as expected. (i.e. giving a
warning where the user wrote the code).
Adding in some comments to clarify your other questions.
http://reviews.llvm.org/D11572
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
