[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

I have some minor comments but overall I think this is in good shape. It would be great to see some compile-time numbers just to make sure this is tractable. I'm pretty sure -fsanitize=null would fire more often across a codebase than this check, so I don't anticipate a big surprise here.



================
Comment at: lib/CodeGen/CGExprScalar.cpp:222
+
+  // The stack of currently-visiting Cast expressions.
+  llvm::SmallVector<CastExpr *, 8> CastExprStack;
----------------
It would help to have this comment explain that the stack is used/maintained exclusively by the implicit cast sanitizer.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:228
+
+    // We only care if we are to use this data.
+    bool Enabled() {
----------------
Could you make this comment more specific -- maybe by explaining that for efficiency reasons, the cast expr stack is only maintained when a sanitizer check is enabled?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:234
+  public:
+    CastExprStackGuard(ScalarExprEmitter *SEE, CastExpr *CE) : SEE(SEE) {
+      if (!Enabled())
----------------
I think if you were to use references instead of pointers here, the code would be a bit clearer, and you wouldn't need to assert that CE is non-null.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:351
+    ScalarConversionOpts()
+        : TreatBooleanAsSigned(false),
+          EmitImplicitIntegerTruncationChecks(false) {}
----------------
Why not use default member initializers here (e.g, "bool a = false")?


================
Comment at: lib/CodeGen/CGExprScalar.cpp:988
+          return Child == PreviousCast;
+        }))
+      return false;
----------------
The none_of call could safely be replaced by `Cast->getSubExpr() != PreviousCast`, I think.


Repository:
  rC Clang

https://reviews.llvm.org/D48958