Endilll wrote: > > Unfortunately, I think this should be held off until we have a bigger > > picture for the future of `Sema`. I'll elaborate below. > > If you take a close look at the existing set of `Sema` parts, it's almost > > entirely comprised of other languages or language extensions (from C and > > C++ standpoint) and backends. They were considered natural enough to be > > split off `Sema`, which, among other things, means that it's easy to > > explain their place in the big picture, like I did in the previous sentence. > > As noted above `-fbounds-safety` **is a C language extension** which makes it > seem like it would fit nicely into the existing division of Sema into > multiple objects and relevant source files.
No, it don't fit nicely into the division, which is the reason we're having this discussion. > > I don't think this patch makes it easier to explain people where things are > > in this codebase, because (I think) `counted_by` is a declaration > > attribute, and we already have place for them. Having only one function > > also makes it a very small part of `Sema`, which doesn't sound compelling. > > It doesn't right now because most of `-fbounds-safety` implementation isn't > upstream yet. I simply moved what is **currently upstream**. As we (Apple) > upstream more and more of the implementation it makes a lot sense to try to > keep as much of it in its own source file to: > > * Create a clean separation between `-fbounds-safety` Sema checks and > everything else. This will ultimately make exploring the code easier once a > significant amount of our implementation has been upstreamed. > > * Avoid growing all the existing `Sema*.cpp` files unnecessarily. They > are already huge and we don't want to make the problem worse when there's an > easy option available to avoid that. You can have `SemaBoundsSafety.cpp` and your own "section" inside `Sema.h`, like C++ features do (like templates or lambdas). There's no reason to make separation physical by introducing `SemaBoundsSafety` class from the get-go. > > A bigger picture this fits in would be a compelling argument (like it was > > for small backend parts), but to my knowledge there's none. > > The `counted_by` attribute is just one of several attributes that are part of > the `-fbounds-safety` language extension which is documented here > https://clang.llvm.org/docs/BoundsSafety.html. That's "bigger picture" for > the feature, or did you mean something else by "bigger picture this fits in"? A bigger picture is "what are we going to do with the rest of the attributes in `SemaDeclAttr.cpp`?". Even bigger one is "what do we do about SemaDecl, SemaExpr, SemaDeclCXX, and SemaExprCXX?" > > Coming up with one and getting maintainers on board is certainly out of > > scope of this PR, so don't feel obligated to do that. Hopefully I'll get > > back to this whole topic later. > > My end goal is to have "somewhere" to put all of the Sema code that supports > `-fbounds-safety` that we will upstream. I think it's very much preferable to > have this code go into its own source file and header file for the reasons I > gave above. > > We don't have to use the sub-class `SemaBase` mechanism that I've used here > if this is really a problem. However, the mechanism does seem like a good fit > for `-fbounds-safety` and would be a shame not to use it. > > "Extension" is definitely quite broad. What I meant are (basically) > > languages that are based off C or C++. Would you argue that > > `-fbounds-safety` fits into a set of OpenMP, OpenACC, CUDA, HLSL, > > Objective-C, and Swift? > > In my opinion it fits in the set because it is a (pretty large) C language > extension. > > > > It's currently in the process of being upstreamed which is why it's > > > small, but it'd be easier to split now than wait until it's reached a > > > certain size, if we do want to split it eventually. > > > > > > I did that a lot with other parts of `Sema`, and it's not that hard, unless > > it grows comparable to `SemaExprCXX.cpp` (which I consider unlikely to > > happen). I'd rather see this being upstreamed into the existing `Sema` > > structure as it has been planned all along, and use it as an input for the > > design of further `Sema` splitting, rather than committing today that > > `SemaBoundsSafety` is going to be a thing. > > I don't agree with this approach. I outlined above why I think it makes a lot > of sense to keep the `-fbounds-safety` Sema code in its own source file and > blocking doing that on a _potential future refactor_ that might never happen > doesn't seem like the right approach to me. I have to say that `-fbounds-safety` upstreaming is in the same boat of something that might never happen. > If at some point we come up with some new design for Sema we can easily move > the `-fbounds-safety` code out of its own source file (and class) and into > the relevant locations required by the redesign. If the `-fbounds-safety` > code is littered all over the other Sema files the _potential future > refactor_ could be much harder because finding all `-fbound-safety` code now > is much more time consuming. That's why I propose to follow long-established practice of doing `SemaBoundsSafety.cpp`, and move that around later. What I'd like to evaluate before deciding on `SemaBoundsChecking` is how big its interface is (what would be exposed via `SemaBoundsChecking` class,) https://github.com/llvm/llvm-project/pull/98954 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
