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.

  rC Clang


cfe-commits mailing list

Reply via email to