On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek <kli...@google.com> wrote:
> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper <djas...@google.com> wrote: > >> So, back in the day when we implemented the matchers, we decided on >> actually wanting to remove all the CXX... AST nodes (there are more of >> them). >> > > Note that Richard has paddled back on this and now says the CXX... AST > nodes are the right thing. > > >> I don't know how this would work as recordDecl already exists. But I'd be >> somewhat hesitant to introduce a cxxRecordDecl matcher if there is still a >> chance that we want to move away from the CXX prefix. >> > > See above. > So do we change all the others at the same time, e.g. create a cxxConstructExpr() matcher? There it make less sense as there is no ConstructExpr, just CXXConstructExpr. Also note that AST matchers are used massively in the wild by now and I >> would be very hesitant to make a change breaking backwards compatibility. >> 85 failures in the clang repositories themselves sounds scary to me. Not >> saying we shouldn't do it at all. But we should be very clear on where >> things should be in the long run. >> > > Aaron has clarified that that's only 14 outside the AST matcher tests > themselves. > Sure, still a lot IMO :(. But ok, maybe there is just no other way. > >> On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman <aa...@aaronballman.com> >> wrote: >> >>> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek <kli...@google.com> >>> wrote: >>> > >>> > >>> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman <aa...@aaronballman.com> >>> wrote: >>> >> >>> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek <kli...@google.com> >>> wrote: >>> >> > >>> >> > >>> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman <aa...@aaronballman.com >>> > >>> >> > wrote: >>> >> >> >>> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek <kli...@google.com> >>> >> >> wrote: >>> >> >> > >>> >> >> > >>> >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman >>> >> >> > <aa...@aaronballman.com> >>> >> >> > wrote: >>> >> >> >> >>> >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith >>> >> >> >> <rich...@metafoo.co.uk> >>> >> >> >> wrote: >>> >> >> >> > I don't think CXXRecordDecl is an anachronism, so much as an >>> >> >> >> > implementation >>> >> >> >> > detail; it makes sense to use a smaller class when in C mode, >>> as >>> >> >> >> > we >>> >> >> >> > don't >>> >> >> >> > need most of the features and complexity that CXXRecordDecl >>> brings >>> >> >> >> > with >>> >> >> >> > it. >>> >> >> >> > But... as a user of clang matchers, I don't think I'd want to >>> care >>> >> >> >> > about >>> >> >> >> > the >>> >> >> >> > difference, and it'd be more convenient if I could nest (say) >>> a >>> >> >> >> > hasMethod >>> >> >> >> > matcher within a recordDecl matcher, since it's completely >>> obvious >>> >> >> >> > what >>> >> >> >> > that >>> >> >> >> > should mean. If I have a matcher that says: >>> >> >> >> > >>> >> >> >> > recordDecl(or(hasMethod(...), hasField(...))) >>> >> >> >> > >>> >> >> >> > I would expect that to work in both C and C++ (and the only >>> way it >>> >> >> >> > could >>> >> >> >> > match in C would be on a record with the specified field, >>> since >>> >> >> >> > the >>> >> >> >> > hasMethod matcher would always fail). >>> >> >> >> >>> >> >> >> Okay, so then it sounds like we want recordDecl to *mean* >>> >> >> >> RecordDecl, >>> >> >> >> but we want the traversal and narrowing matchers that currently >>> take >>> >> >> >> a >>> >> >> >> CXXRecordDecl to instead take a RecordDecl and handle the CXX >>> part >>> >> >> >> transparently? This means we would not need to add a >>> cxxRecordDecl() >>> >> >> >> matcher, but could still access CXX-only functionality (like >>> access >>> >> >> >> control, base classes, etc) through recordDecl()? >>> >> >> > >>> >> >> > >>> >> >> > I'm against that proposal. I think we have tried to make the >>> matchers >>> >> >> > more >>> >> >> > "user friendly" in the past, and all those attempts have failed >>> >> >> > miserably; >>> >> >> > in the end, users will do ast-dump to see what they want to >>> match, >>> >> >> > and >>> >> >> > then >>> >> >> > be confused when the matchers do follow the AST 99% of the time, >>> but >>> >> >> > try >>> >> >> > to >>> >> >> > be smart 1% of the time. >>> >> >> > I think given that we want to keep CXXRecordDecl, the right >>> solution >>> >> >> > is >>> >> >> > to >>> >> >> > have a cxxRecordDecl() matcher. >>> >> >> >>> >> >> Personally, I think this makes the most sense, at least to me. The >>> >> >> recommendation I've always heard (and given) is to use -ast-dump >>> and >>> >> >> write matchers from there. (Consequently, the more I work with type >>> >> >> traversal matchers, the more I wish we had -ast-dump-types to give >>> >> >> even *more* information for writing matchers.) >>> >> >> >>> >> >> But the question still remains, what do we do with recordDecl? >>> Right >>> >> >> now, it means CXXRecordDecl instead of RecordDecl. If we change it >>> to >>> >> >> mean RecordDecl instead, there's a chance we'll break existing, >>> >> >> reasonable code. Are we okay with that risk? If we're at least >>> >> >> conceptually okay with it, I could make the change locally and see >>> >> >> just how much of our own code breaks, and report back. But if that >>> >> >> turns out to be problematic, do we want to deprecate recordDecl and >>> >> >> replace it with structDecl as our fallback position? Or is there a >>> >> >> better solution? >>> >> >> >>> >> >> Basically, I see a few ways to solve this (and there may be other >>> ways >>> >> >> I'm not thinking about yet): >>> >> >> >>> >> >> 1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and >>> >> >> cxxRecordDecl. This does not match the AST because we have no >>> >> >> StructDecl or UnionDecl types. The more I think about this option, >>> the >>> >> >> less I like it. It's easy to implement, but seems hard to relate to >>> >> >> the AST. >>> >> >> 2) Make recordDecl match RecordDecl, don't touch other matchers. >>> Add >>> >> >> way to distinguish unions from structs (e.g., isUnion(), >>> isStruct()), >>> >> >> add cxxRecordDecl. This matches the AST most closely, but may break >>> >> >> code. I think that I prefer this approach, but it depends heavily >>> on >>> >> >> what "may break code" looks like in practice. >>> >> >> 3) Make recordDecl match RecordDecl, fix other matchers that >>> currently >>> >> >> take CXXRecordDecl to instead take RecordDecl and handle sensibly >>> when >>> >> >> possible. Add a way to distinguish unions from structs, add >>> >> >> cxxRecordDecl. This doesn't match the AST because we will have >>> >> >> matchers taking a RecordDecl when the AST would require a >>> >> >> CXXRecordDecl, but is likely to break less code. >>> >> > >>> >> > >>> >> > That sums it up. My preferences are 2, 3, 1 in that order :) >>> >> >>> >> I've attached a patch that implements #2, but it comes with ~85 errors >>> >> from C++ matchers that use recordDecl to mean cxxRecordDecl. >>> >> >>> >> http://pastebin.com/bxkRcqBV >>> >> >>> >> If this is an acceptable failure rate, I can also update the failing >>> >> matchers to use cxxRecordDecl instead of recordDecl where applicable. >>> >> Doing some spot-checking of the failing code, the failures are ones we >>> >> anticipated, such as: >>> >> >>> >> constructorDecl(ofClass(recordDecl( >>> >> hasDeclaration(recordDecl(hasMethod(hasName("begin")), >>> >> hasMethod(hasName("end")))) >>> >> etc >>> > >>> > >>> > +Daniel for another opinion. I think this is fine, but I'd prefer not >>> to >>> > end up in a meme :) >>> >>> FWIW, the vast majority of the errors are in ASTMatchersTests. There >>> were only a few in actual real-world uses (71 in tests, 14 in real >>> code). >>> >>> ~Aaron >>> >>> > >>> >> ~Aaron >>> >> >>> >> > >>> >> >> >>> >> >> ~Aaron >>> >> >> >>> >> >> >>> >> >> > Richard: if CXXRecordDecl was really an implementation detail, it >>> >> >> > would >>> >> >> > be >>> >> >> > hidden behind a RecordDecl class, as an implementation detail. >>> The >>> >> >> > reasons >>> >> >> > why we don't want it to be an implementation detail in the code >>> >> >> > (performance, data structure size) don't matter - in the end, >>> it's in >>> >> >> > the >>> >> >> > AST API. >>> >> >> > >>> >> >> >> >>> >> >> >> >>> >> >> >> ~Aaron >>> >> >> >> >>> >> >> >> > >>> >> >> >> > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek < >>> kli...@google.com> >>> >> >> >> > wrote: >>> >> >> >> >> >>> >> >> >> >> Richard! We need an informed opinion :D >>> >> >> >> >> >>> >> >> >> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman >>> >> >> >> >> <aa...@aaronballman.com> >>> >> >> >> >> wrote: >>> >> >> >> >>> >>> >> >> >> >>> Ping? >>> >> >> >> >>> >>> >> >> >> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek >>> >> >> >> >>> <kli...@google.com> >>> >> >> >> >>> wrote: >>> >> >> >> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman >>> >> >> >> >>> > <aa...@aaronballman.com> >>> >> >> >> >>> > wrote: >>> >> >> >> >>> >> >>> >> >> >> >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek >>> >> >> >> >>> >> <kli...@google.com> >>> >> >> >> >>> >> wrote: >>> >> >> >> >>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman >>> >> >> >> >>> >> > <aa...@aaronballman.com> >>> >> >> >> >>> >> > wrote: >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek >>> >> >> >> >>> >> >> <kli...@google.com> >>> >> >> >> >>> >> >> wrote: >>> >> >> >> >>> >> >> > Yea, we had that discussion a few times, and I can >>> never >>> >> >> >> >>> >> >> > remember >>> >> >> >> >>> >> >> > why >>> >> >> >> >>> >> >> > we >>> >> >> >> >>> >> >> > ended up in the state we're in. >>> >> >> >> >>> >> >> > We definitely had a time where we switched to just >>> using >>> >> >> >> >>> >> >> > the >>> >> >> >> >>> >> >> > exact >>> >> >> >> >>> >> >> > same >>> >> >> >> >>> >> >> > name >>> >> >> >> >>> >> >> > as the node's class name for the matchers. >>> >> >> >> >>> >> >> > I *think* we didn't do it for cxxRecordDecl, because >>> >> >> >> >>> >> >> > Richard >>> >> >> >> >>> >> >> > said >>> >> >> >> >>> >> >> > that's >>> >> >> >> >>> >> >> > a >>> >> >> >> >>> >> >> > relic we should get rid of anyway, but I'm not sure. >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> FWIW, I think the state we're in is the worst of all >>> >> >> >> >>> >> >> worlds. >>> >> >> >> >>> >> >> It's >>> >> >> >> >>> >> >> not >>> >> >> >> >>> >> >> intuitive that recordDecl() doesn't match a struct in >>> C >>> >> >> >> >>> >> >> mode, >>> >> >> >> >>> >> >> and >>> >> >> >> >>> >> >> as >>> >> >> >> >>> >> >> it stands, there is no way to match a struct or union >>> >> >> >> >>> >> >> declaration >>> >> >> >> >>> >> >> in C >>> >> >> >> >>> >> >> at all. >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > Agreed. Best intentions. Worst possible outcome. That's >>> >> >> >> >>> >> > software >>> >> >> >> >>> >> > development >>> >> >> >> >>> >> > :) >>> >> >> >> >>> >> > >>> >> >> >> >>> >> >> > >>> >> >> >> >>> >> >> > On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman >>> >> >> >> >>> >> >> > <aa...@aaronballman.com> >>> >> >> >> >>> >> >> > wrote: >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> It turns out that the recordDecl() AST matcher >>> doesn't >>> >> >> >> >>> >> >> >> match >>> >> >> >> >>> >> >> >> RecordDecl objects; instead, it matches >>> CXXRecordDecl >>> >> >> >> >>> >> >> >> objects. >>> >> >> >> >>> >> >> >> This >>> >> >> >> >>> >> >> >> is... unfortunate... as it makes writing AST >>> matchers >>> >> >> >> >>> >> >> >> more >>> >> >> >> >>> >> >> >> complicated >>> >> >> >> >>> >> >> >> because of having to translate between >>> >> >> >> >>> >> >> >> recordDecl()/CXXRecordDecl. >>> >> >> >> >>> >> >> >> It >>> >> >> >> >>> >> >> >> also makes it impossible to match a struct or union >>> >> >> >> >>> >> >> >> declaration >>> >> >> >> >>> >> >> >> in C >>> >> >> >> >>> >> >> >> or ObjC. However, given how prevalent >>> recordDecl()'s >>> >> >> >> >>> >> >> >> use >>> >> >> >> >>> >> >> >> is >>> >> >> >> >>> >> >> >> in >>> >> >> >> >>> >> >> >> the >>> >> >> >> >>> >> >> >> wild (I'm guessing), changing it at this point >>> would be >>> >> >> >> >>> >> >> >> a >>> >> >> >> >>> >> >> >> Bad >>> >> >> >> >>> >> >> >> Thing. >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> For people trying to write AST matchers for >>> languages >>> >> >> >> >>> >> >> >> like >>> >> >> >> >>> >> >> >> C >>> >> >> >> >>> >> >> >> or >>> >> >> >> >>> >> >> >> ObjC, >>> >> >> >> >>> >> >> >> I would like to propose adding: >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> structDecl() >>> >> >> >> >>> >> >> >> unionDecl() >>> >> >> >> >>> >> >> >> tagDecl() >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> These will match nicely with the existing >>> enumDecl() >>> >> >> >> >>> >> >> >> AST >>> >> >> >> >>> >> >> >> matcher. >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> Additionally, I would like to add cxxRecordDecl() >>> to >>> >> >> >> >>> >> >> >> match >>> >> >> >> >>> >> >> >> CXXRecordDecl objects. While it duplicates the >>> >> >> >> >>> >> >> >> functionality >>> >> >> >> >>> >> >> >> exposed >>> >> >> >> >>> >> >> >> by recordDecl(), it more clearly matches the >>> intention >>> >> >> >> >>> >> >> >> of >>> >> >> >> >>> >> >> >> which >>> >> >> >> >>> >> >> >> AST >>> >> >> >> >>> >> >> >> node it corresponds to. >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> Finally, I would like to undocument recordDecl() >>> and >>> >> >> >> >>> >> >> >> change >>> >> >> >> >>> >> >> >> our >>> >> >> >> >>> >> >> >> existing documentation and AST matcher uses to use >>> >> >> >> >>> >> >> >> cxxRecordDecl/structDecl() instead. Maybe someday >>> we >>> >> >> >> >>> >> >> >> can >>> >> >> >> >>> >> >> >> deprecate >>> >> >> >> >>> >> >> >> recordDecl() more officially. >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> I'm open to other ideas if there are better ways to >>> >> >> >> >>> >> >> >> move >>> >> >> >> >>> >> >> >> forward. If >>> >> >> >> >>> >> >> >> you think changing the meaning of recordDecl() is >>> >> >> >> >>> >> >> >> acceptable, >>> >> >> >> >>> >> >> >> I >>> >> >> >> >>> >> >> >> can >>> >> >> >> >>> >> >> >> also go that route (though I would still propose >>> adding >>> >> >> >> >>> >> >> >> unionDecl() >>> >> >> >> >>> >> >> >> and cxxRecordDecl() in that case). >>> >> >> >> >>> >> >> > >>> >> >> >> >>> >> >> > >>> >> >> >> >>> >> >> > I think changing recordDecl is acceptable. I believe >>> >> >> >> >>> >> >> > very >>> >> >> >> >>> >> >> > few >>> >> >> >> >>> >> >> > tools >>> >> >> >> >>> >> >> > will >>> >> >> >> >>> >> >> > actually start doing wrong things because of it. I'd >>> >> >> >> >>> >> >> > like >>> >> >> >> >>> >> >> > more >>> >> >> >> >>> >> >> > opinions >>> >> >> >> >>> >> >> > first, though :) >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> I was giving this more thought over the long weekend, >>> and >>> >> >> >> >>> >> >> I >>> >> >> >> >>> >> >> think >>> >> >> >> >>> >> >> you >>> >> >> >> >>> >> >> may be right. I think changing recordDecl() to mean >>> >> >> >> >>> >> >> RecordDecl >>> >> >> >> >>> >> >> will >>> >> >> >> >>> >> >> fix more code than it breaks, so long as we take a >>> >> >> >> >>> >> >> holistic >>> >> >> >> >>> >> >> approach >>> >> >> >> >>> >> >> to the change and see which narrowing and traversal >>> >> >> >> >>> >> >> matchers >>> >> >> >> >>> >> >> we >>> >> >> >> >>> >> >> need >>> >> >> >> >>> >> >> to fix up at the same time. When I tried to think of >>> AST >>> >> >> >> >>> >> >> matchers >>> >> >> >> >>> >> >> that >>> >> >> >> >>> >> >> mean CXXRecordDecl but *not* RecordDecl, they were >>> >> >> >> >>> >> >> horribly >>> >> >> >> >>> >> >> contrived >>> >> >> >> >>> >> >> because you usually are matching on additional >>> selection >>> >> >> >> >>> >> >> criteria >>> >> >> >> >>> >> >> that >>> >> >> >> >>> >> >> is specific to C++ (such as hasMethod() or >>> >> >> >> >>> >> >> isDerivedFrom()) >>> >> >> >> >>> >> >> which >>> >> >> >> >>> >> >> would cause the match to continue to fail, as >>> expected. >>> >> >> >> >>> >> >> Code >>> >> >> >> >>> >> >> that >>> >> >> >> >>> >> >> uses >>> >> >> >> >>> >> >> recordDecl() to mean RecordDecl will suddenly start to >>> >> >> >> >>> >> >> match >>> >> >> >> >>> >> >> in >>> >> >> >> >>> >> >> more >>> >> >> >> >>> >> >> cases, but that's likely to be a bug fix more than a >>> >> >> >> >>> >> >> breaking >>> >> >> >> >>> >> >> change. >>> >> >> >> >>> >> >> To the best of my understanding, the only breaking >>> cases >>> >> >> >> >>> >> >> would >>> >> >> >> >>> >> >> be >>> >> >> >> >>> >> >> where you wrote recordDecl(), meant CXXRecordDecl, >>> had no >>> >> >> >> >>> >> >> further >>> >> >> >> >>> >> >> narrowing or traversal matchers, and were compiling >>> in C >>> >> >> >> >>> >> >> mode; >>> >> >> >> >>> >> >> with >>> >> >> >> >>> >> >> the result being additional unexpected matches. >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > Ah, there's one thing that can break: the compile can >>> >> >> >> >>> >> > break: >>> >> >> >> >>> >> > recordDecl(hasMethod(...)) will *not* compile (it'll >>> work >>> >> >> >> >>> >> > in >>> >> >> >> >>> >> > the >>> >> >> >> >>> >> > dynamic >>> >> >> >> >>> >> > matchers and fail as you suggest, but the in-C++ DSL >>> does >>> >> >> >> >>> >> > more >>> >> >> >> >>> >> > static >>> >> >> >> >>> >> > type >>> >> >> >> >>> >> > checking). >>> >> >> >> >>> >> > I don't think that's super bad though. >>> >> >> >> >>> >> > >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> So perhaps it would make sense to: >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> 1) Make recordDecl() mean RecordDecl >>> >> >> >> >>> >> >> 2) Do a comprehensive review of matchers that take a >>> >> >> >> >>> >> >> CXXRecordDecl >>> >> >> >> >>> >> >> and >>> >> >> >> >>> >> >> see if they should instead take a RecordDecl >>> >> >> >> >>> >> >> 3) Add unionDecl() as a node matcher (or should we add >>> >> >> >> >>> >> >> isUnion() >>> >> >> >> >>> >> >> and >>> >> >> >> >>> >> >> isStruct() as narrowing matchers?) >>> >> >> >> >>> >> >> 4) Add tagDecl() as a node matcher, but not add >>> >> >> >> >>> >> >> cxxRecordDecl() >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > >>> >> >> >> >>> >> > Why not add cxxRecordDecl()? I think we need it if we >>> want >>> >> >> >> >>> >> > narrowing >>> >> >> >> >>> >> > matchers on CXXRecordDecl? >>> >> >> >> >>> >> >>> >> >> >> >>> >> If Richard thinks CXXRecordDecl is an anachronism, I >>> figured >>> >> >> >> >>> >> we >>> >> >> >> >>> >> didn't >>> >> >> >> >>> >> want to expose it. Instead, we could make hasMethod (et >>> al) >>> >> >> >> >>> >> accept >>> >> >> >> >>> >> a >>> >> >> >> >>> >> RecordDecl and do the type checking for the caller. Then >>> >> >> >> >>> >> recordDecl(hasMethod(...)) continues to compile and >>> work, and >>> >> >> >> >>> >> when >>> >> >> >> >>> >> hasMethod is given a RecordDecl instead of a >>> CXXRecordDecl, >>> >> >> >> >>> >> it >>> >> >> >> >>> >> simply >>> >> >> >> >>> >> matches nothing. But you bring up a good point about the >>> C++ >>> >> >> >> >>> >> DSL >>> >> >> >> >>> >> being >>> >> >> >> >>> >> a problem still, I hadn't considered that. >>> >> >> >> >>> > >>> >> >> >> >>> > >>> >> >> >> >>> > First I want Richard to confirm that. I have a very bad >>> >> >> >> >>> > memory, >>> >> >> >> >>> > so I >>> >> >> >> >>> > might >>> >> >> >> >>> > as well misremember :) >>> >> >> >> >>> > >>> >> >> >> >>> >> >>> >> >> >> >>> >> >>> >> >> >> >>> >> ~Aaron >>> >> >> >> >>> >> >>> >> >> >> >>> >> > >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> ~Aaron >>> >> >> >> >>> >> >> >>> >> >> >> >>> >> >> > >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> Thanks! >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> >> ~Aaron >>> >> >> >> > >>> >> >> >> > >>> >> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
