Add the ability to use the children() range API in a const-correct manner
ClosedPublic

Authored by aaron.ballman on Mar 20 2017, 3:30 PM.

Details

Summary

We already have children() but not children() const, which is causing some problems for me on an out of tree project. Most of the changes here are mechanical, except for the work done to ConstStmtIterator, which cheats and uses a const_cast. Since none of these are *actually* const, this should not trigger UB.

Diff Detail

aaron.ballman created this revision.Mar 20 2017, 3:30 PM

Missed a case for UnaryExprOrTypeTraitExpr.

Ping.

Because I expect this to be fairly uncontroversial, I'll give it a week in case folks have comments, then commit (and will handle anything in post-commit review).

As I mentioned to Craig Topper recently on another review, generally when implementing const and non-const overloads the non-const is implemented in terms of the const overload (& const_casts away const on the result). This ensures no UB if the const overload is called on a truly const object. Even if there aren't any truly const Stmts today, I'd still prefer the code be written so as not to assume they couldn't exist.

As I mentioned to Craig Topper recently on another review, generally when implementing const and non-const overloads the non-const is implemented in terms of the const overload (& const_casts away const on the result). This ensures no UB if the const overload is called on a truly const object. Even if there aren't any truly const Stmts today, I'd still prefer the code be written so as not to assume they couldn't exist.

In practice, this is really hard to do because there's no such thing as a const_iterator_cast operation to convert a const_iterator into an iterator, unless you have the underlying container handy. Since many of these uses do not have an underlying container (they're often trailing objects), I'm not certain this is practical (though I agree with the reasoning). Suggestions welcome, however.

Addressing David's review feedback.

dblaikie added inline comments.Tue, Apr 11, 9:43 AM
include/clang/AST/Expr.h
4025

If this is adding const, can you use a weaker cast? (I guess what would be ideal here would be implicit_cast - if you're interested you could add that - otherwise... yeah, I guess static_cast could do other nasty things, etc, so there's nothing good)

include/clang/AST/StmtIterator.h
148–150

Maybe I'm missing something - what stops code from doing this conversion implicitly/accidentally all over the place? (I would've expecetd cast_away_const to be a friend or something, to only allow the conversion through this explicit call/operation)

Addressing review comments.

include/clang/AST/Expr.h
4025

I'm not certain what you mean. This is adding the const qualifier so that we call the proper overload. I'm not certain of what a weaker cast would be.

include/clang/AST/StmtIterator.h
148–150

Good catch, I've corrected this.

dblaikie accepted this revision.Tue, Apr 11, 10:44 AM

Looks good to me - thanks!

include/clang/AST/StmtIterator.h
137

the "struct" here is a bit atypical/should be removed

This revision is now accepted and ready to land.Tue, Apr 11, 10:44 AM
aaron.ballman added inline comments.Tue, Apr 11, 10:51 AM
include/clang/AST/StmtIterator.h
137

The forward reference is needed because ConstStmtIterator is defined below StmtIterator. If I use class rather than struct, then MSVC diagnoses because of the mismatched class-key (which is a pretty low-value diagnostic, but I see no reason to disable it either).

aaron.ballman closed this revision.Tue, Apr 11, 1:34 PM

Commit in r299981.