[clang] [clang][NFC] Regroup declarations in `Sema` (PR #82217)

Erich Keane via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 20 07:39:18 PST 2024


erichkeane wrote:

> This is going to be rather disruptive on downstream projects. At least we should wait until after the release of clang 18 to merge it, to avoid endless merge conflicts

For the most part, git will handle these pretty well on downstreams I think, and as they are declarations, I suspect this isn't going to be super tough of a merge conflict.  That said, waiting until after 18 is perhaps a good diea.

>  Table of contents added at the very beginning of `Sema`. Grouping is reflected in Doxygen commands, so structure of API reference of `Sema` is also significantly improved ([example from official documentation](https://www.doxygen.nl/manual/examples/memgrp/html/class_memgrp___test.html)).

Neat!
> 
> While grouping is intentional, as well as each group consisting of `public` declarations followed by `private` ones (without changing access in-between),

I MIGHT suggest private followed by public?  It is a not-uncommon pattern I've seen to have a private 'helper' class(or data member) defined inline, that is then used by inline-defined public functions.  

>Data members and inline function definitions in `Sema.h` complicate the matter, since it's not obvious which group they belong to. Further work is expected to refine contents and order of declarations.

This IS going to be complicated unfortunately.  Many of the function definitions at least are going to be 1-liners that are just calling into a different definition.  Data members you'll probably have to hunt down where they are used to see the relationship.

> What is also intentional is some kind of layering, where Concepts group follows template groups, and ObjC, code completion, CUDA, HLSL, OpenACC, OpenMP, and SYCL are all placed at the end of the file, after C and C++ parts of `Sema`.

 
> Member initializer list of `Sema` in `Sema.cpp` is rewritten to reflect new order of data members in order to avoid `-Wreorder-ctor`.

I wouldn't mind some sort of 'static_assert' to ensure that this doesn't accidentally increase the size of Sema or cause some sort of pessimization for layout.  I realize we're not particularly concerned about the size, but I could imagine goofy things going on.  

https://github.com/llvm/llvm-project/pull/82217


More information about the cfe-commits mailing list