LLVM 13 support #21
LLVM 13 support #21
Conversation
…rMain.cpp,CodeStyleCheckerMain.cpp,LACommenterMain.cpp}] LLVM 13
|
Many thanks for submitting this! I've left a few suggestions inline. We will also have to update the CI files. Would you be able to do that, pls? -Andrzej |
…, and Ninja; [.editorconfig] Init; [lib/CodeRefactor.cpp] Use `size_t` for array size index type; use .empty() for string size == 0 check
|
Done. Where are the inline suggestions? |
… use LLVM error stream; as per Expected docs (& as per new factory from https://reviews.llvm.org/D39042 that the CLI parser returns); [.gitignore] Ignore IDE and build dir
| COMMAND chmod +x ninja | ||
| ) | ||
| endif() | ||
| git clone git:https://github.com/martine/ninja.git |
There was a problem hiding this comment.
Hm, I think that you meant https://github.com/ninja-build/ninja?
There was a problem hiding this comment.
Ohhh the guide still hasn't taken in my patch? - https://clang.llvm.org/docs/HowToSetupToolingForLLVM.html#using-ninja-build-system to https://reviews.llvm.org/D98594 - let me send them a new patch to include the ninja-build org
There was a problem hiding this comment.
Sent them a patch: https://reviews.llvm.org/D111490
There was a problem hiding this comment.
Looking really good, thanks!
-
Is
.editorconfiga VS Code config file? Was it added by accident? -
There are a few unrelated changes here, i.e. changes that are not required for the switch to LLVM 13. My personal preference would be to move them to a dedicated patch. But I don't want to create extra work for you. Instead, could you add a note in the commit message that summarises the additional improvements? This way the git history will make it clear that this was intentional. Otherwise, it's unrelated and confusing.
-
Btw, most of these "unrelated changes" make a lot of sense - thanks for the improvements!
Before merging, I suggest that you squash all your changes and force push to your branch. Basically, I try to stick to one commit per PR and no merge-commits. Just to keep the history simple. I could squash your commits in the GitHub UI myself, but I think that it's nicer if you are in full control of the commit message.
|
…Parser` => `eOptParser`
|
|
@banach-space Sure I don't care, as long as it's a format that IDEs and editors recognise (and it is). |
|
LGTM, let's merge this! Shall I squash this with the web UI? Or do you prefer to do it yourself? |
|
Great, merge away (squash in web UI is fine) |
After #21, the following is no longer correct nor required: ```CMake -D CMAKE_MAKE_PROGRAM=$GITHUB_WORKSPACE/ninja ```
After #21, the following is no longer correct nor required: ```CMake -D CMAKE_MAKE_PROGRAM=$GITHUB_WORKSPACE/ninja ```
Closes #20
WiP I guess