rjmccall added inline comments.
================ Comment at: lib/Sema/SemaExpr.cpp:8808 + Context.getTypeSize(pointerType) == + Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; ---------------- efriedma wrote: > andrew.w.kaylor wrote: > > efriedma wrote: > > > rsmith wrote: > > > > andrew.w.kaylor wrote: > > > > > efriedma wrote: > > > > > > Please make sure you use exactly the same check in Sema and CodeGen > > > > > > (probably easiest to stick a helper into lib/AST/). > > > > > That's a good idea, but I'm not really familiar enough with the > > > > > structure of clang to be sure I'm not doing it in a ham-fisted way. > > > > > Can you clarify? Are you suggesting adding something like > > > > > ASTContext::isPointeeTypeCharSize() and > > > > > ASTContext::isIntegerTypePointerSize()? Or maybe a single > > > > > specialized function somewhere that does both checks like > > > > > ASTContext::areOperandNullPointerArithmeticCompatible()? > > > > I would suggest something even more specific, such as > > > > `isNullPointerPlusIntegerExtension` > > > I'm most concerned about the difference between > > > "isa<llvm::ConstantPointerNull>(pointer)" and > > > "PExp->IgnoreParenCasts()->isNullPointerConstant()"... they're different > > > in important ways. > > > > > > I was thinking something like > > > "BinaryOperator::isNullPointerArithmeticExtension()" > > At this point in Sema the BinaryOperator for the addition hasn't been > > created yet, right? So a static function that takes the opcode and > > operands? > I wasn't really thinking about that... but yes, something like that. I've long thought that it would make sense to store a semantic operation kind in BinaryOperator and UnaryOperator instead of treating the operator alone as a sufficient discriminator. Of course the operator is *usually* sufficient, but there are cases where that's not true — for example, we could use that operation kind to distinguish integer and pointer arithmetic, and maybe even to identify which side of a + is the pointer. If we had that, we could very easily flag a null+int operation as a completely different semantic operation, which it basically is. https://reviews.llvm.org/D37042 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
