https://github.com/NagyDonat commented:

Thanks for the commit!

Then the change looks promising (assuming that the tests are passing – I didn't 
look at the implementation yet) but I'm surprised by the sheer size of the new 
code, especially coming from a new contributor. If you used AI tools, please 
[follow our policy](https://llvm.org/docs/AIToolPolicy.html) and declare this 
fact.

Also, based on the amount of new code, I would prefer if you moved the "bulk" 
of your change to separate source files `ContainerOfModeling.h` (which just 
declares `getContainerOfParentRegion` and is included in 
`ArrayBoundChecker.cpp`) and `ContainerOfModeling.cpp` (which contains the 
definitions).

You can follow the example of my PR 
https://github.com/llvm/llvm-project/pull/213957 which also "splits off" logic 
into separate files. (In my PR I introduce a namespace `clang::ento::bounds` to 
group several methods together. As your ``ContainerOfModeling.h` would contain 
only one function declaration, you should put it directly into `clang::ento`.)

By the way, the github CI cannot execute the tests, because you have merge 
conflicts, probably with my recent changes in the ArrayBound checker. Please 
merge `main` into your branch and manually resolve the conflicts.

https://github.com/llvm/llvm-project/pull/214140
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to