Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
thanks for doing this work.
any pointers to read about GN?
Also please include code owners as reviewers next time.
LGTM
llvm/utils/gn/secondary/llvm/lib/Target/BPF/MCTargetDesc/BUILD.gn | ||
---|---|---|
39 ↗ | (On Diff #184246) | Nit: newline between targets. |
Re-opening since this was reverted in rL352649.
@yonghong-song this wasn't a change to the BPF backend so I'm not entirely clear on why you think this needed to be reviewed by a BPF code owner?
lgtm too.
@ast @yonghong-song This code isn't in lib/Target/BPF and you're not taking on any maintenance of these new files. This is just a clone of the CMake build files; see git log llvm/utils/gn/secondary/llvm/lib/Target for several other files like this.
I read through the link and yes the code looks okay. I think this is a communication issue.
It would be good next time if you have better commit messages which provides enough info. about the change so reviewers can at least
take a look knowing what happens. In the case the reviewer requested a change, it would be good to send out information (as you did)
and wait for acknowledgement before merging.
hmm. so you're saying when we add new file to the normal CMakeFiles, we don't need to touch GN ?
So it's going to bit rot immediately?
what's the purpose of this GN work then?
Yes.
So it's going to bit rot immediately?
No – turns out files aren't changed that frequently, and people who use the GN build usually fix up the build files pretty quickly. (git log --pretty=oneline --grep Merge llvm/utils/gn/)
what's the purpose of this GN work then?
gn runs 1000x faster than cmake, and it allows things like https://reviews.llvm.org/D56713 . It also has the effect of us reading the cmake files very closely and cleaning them up a bit :-)