Moving named cast helpers out of Sema

Hi,

Attached patch moves the C++ named cast helper functions out of Sema and makes them static functions local to their own file., as requested by Chris. (The patch also moves all named cast stuff to its own file.)

Unfortunately, this requires making a few helper functions that are private in Sema public, so I'm not sure if this patch is a good idea. What do you think?

The critical changes are those to Sema.h. The others are big but harmless.

Sebastian

cast.patch (59.1 KB)

If the methods require the Sema object, I don’t really think that moving them out of Sema makes sense or makes the code better.

Chris?

  • Daniel

Hi,

Attached patch moves the C++ named cast helper functions out of Sema and makes them static functions local to their own file., as requested by Chris. (The patch also moves all named cast stuff to its own file.)

This looks great to me, thanks Sebastian!

Unfortunately, this requires making a few helper functions that are private in Sema public, so I'm not sure if this patch is a good idea. What do you think?

I think it's the right thing to do. The entire Sema class is private to the sema module. C++ doesn't have very fine grained access control. For example, in Java, we'd make a bunch of these things public within the sema package but private outside it. In this case, it doesn't really matter, since all of Sema is private to the sema library (not even in include/clang) so access control is not critical here.

Quoeth Daniel:

If the methods require the Sema object, I don't really think that moving them out of Sema makes sense or makes the code better.

To me, the real issue is that the Sema *class* is really about the implementation of the actions module. Given the shear size of the actions interface (which sema must implement) it is a huge class and lacks any good modular breakdown. I'd much rather have complex/involved subsystems split out as static functions where it makes sense, just to reduce the size of the sema interfaces and make it more clear what is part of the sema interface and part of the sema internal implementation details.

Does this seem reasonable?

-Chris

Chris Lattner wrote:

Hi,

Attached patch moves the C++ named cast helper functions out of Sema and makes them static functions local to their own file., as requested by Chris. (The patch also moves all named cast stuff to its own file.)

This looks great to me, thanks Sebastian!

So, one voice for, one against. Do we have a tie-breaker? Or is Chris a tie-breaker by himself? :wink:

Sebastian

I see both sides. Perhaps what really should be done is to split out a separate class for the actual implementation of Sema? Or alternately, there may be parts of Sema which can be refactored into independent helper classes. I don’t have a good view of Sema as a whole, I have only touched pieces of it in an ad hoc manner.

I don’t think you should get saddled with the work Sebastian, so I would say go ahead and follow Chris’ suggestions and we can push improving Sema’s organization to another topic or a rainy day. :slight_smile:

  • Daniel