[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
----------------
riccibruno wrote:
> steveire wrote:
> > Why remove the `LLVM_READONLY` from this class instead of from everywhere in the file it is not needed? (in a separate commit, obviously).
> `LLVM_READONLY` is a macro for `__attribute__((pure))` (when supported). It is useful in some cases to give a hint to the optimizer about the behavior of a function. However in this case the definition of the function is visible in all translation units using this function. Therefore the optimizer can very well see on its own what this function is doing.
> 
> It would be perfectly possible to inspect all uses of `LLVM_READONLY` and only keep what is needed, but this is such a small issue that nobody bothered to do it until now.
My point was again about commit hygiene.

You could make a commit changing this without further review, leaving this review cleaner. You could do the same with the moved friend decl. Commits which only do one thing are easier to review.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57104/new/

https://reviews.llvm.org/D57104