[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

[Vedant Kumar via Phabricator via cfe-commits]

vsk marked 3 inline comments as done.
vsk added a comment.

Thanks for your feedback. I will updated the patch shortly.



================
Comment at: lib/CodeGen/CGExpr.cpp:1221
+/// Check if \p Ty is defined as BOOL in a system header. In ObjC language
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager &SM,
----------------
ahatanak wrote:
> If your intention is to exclude BOOLs defined in files that aren't system headers, is it possible to add a test for that?
> 
> 
> ```
> void foo() {
>   typedef long long BOOL;
>   ...
> }
> ```
Thinking about this some more, I think it's enough to check for an ObjC language mode, and that the system header check is unnecessary.


================
Comment at: lib/CodeGen/CGExpr.cpp:1222
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager &SM,
+                       const LangOptions &LO) {
----------------
zaks.anna wrote:
> Could you use the existing method for this? From NSAPI.h:
> 
> ```
>    // \brief Returns true if \param T is a typedef of "BOOL" in objective-c.
>    bool isObjCBOOLType(QualType T) const;
> 
> ```
> 
Thanks, this is a lot better than rolling my own.


================
Comment at: lib/CodeGen/CGExpr.cpp:1224
+                       const LangOptions &LO) {
+  if (!LO.ObjC1 && !LO.ObjC2)
+    return false;
----------------
arphaman wrote:
> LangOptions.ObjC1 should be always set if LangOptions.ObjC2 is set, so the second check is redundant I think.
Using NSAPI takes care of this.


https://reviews.llvm.org/D27607