[libclc] Several little QOL improvements to libclc-remangler #13073
Conversation
ExitOnError takes a llvm::Expected, but we were passing it the result of llvm::parseIR - std::unique_ptr<llvm::Module> - which, even when null, is not an error condition. Thus invalid IR input was silently being accepted until it would segfault on accessing the module.
I don't think there's any reason to open it at root. Opening it at the current working directory is more intuitive for developers using relative paths for input/output files. Previously "--input-ir foo.ll" would try and open "/foo.ll", which depending on the system is likely a permissions error, or a missing file, or even an unintended file.
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@steffenlarsen Who do we need to talk to to be able to enable merging without squashing? It's making good contributors' lives hard, and forcing us to make history messier
|
I am not sure I see how and why it makes contributing harder and the history messier by squashing. In fact, I think it makes the history much cleaner, but I would love to discuss the different views on it. Maybe it is a topic for a forum discussion? Tag @bader. For now, feel free to submit separate PRs if having them appear in separation in the history is important. It also makes for some more focused PRs, which is nice for reviewers. Edit; On a related note, putting a note in the description for gatekeepers merging your PR is not always going to make gatekeepers notice. Reviewers and gatekeeper are not always the same, so gatekeepers may miss the message. It is also why adding "[WIP][DO NOT MERGE]" to the title didn't always work and why Draft is such a nice feature. 😉 |
then
That's one of the things that makes developers' lives hard; if you have to get three separate reviews for cleanup patchsets, and then wait for 3 separate CI builds, and interrupt gatekeepers 3 times, then that slows things down at least 3 times. These commits are conceptually grouped, but each do one thing. I've spent enough of my life debugging only to find a commit that did more than its commit message said to know that well factored individual commits are much easier to reason about later.
I brought this up in #8229 |
|
To be clear
was if it really is important to the contributor that these appear in separate commits. If they are closely related, I would argue that it is fine that they appear in the same commit in the history. If they are not closely related, they should also be reviewed in separation. That also parallelizes review, granted for 15 LOC changes parallelizing review doesn't make much sense.
I'll put my view on the subject in there. 👍 |
Sorry, I thought I had read someone explain that this was the policy, but that was either some other repository or out-of-date advice. I don't need these commits to be kept separate - so feel free squash and merge - but I thought I'd ask in case it was possible as it's my preference. I'm disappointed in the policy, but I can take my thoughts to the discussion thread once I formulate them. |
I've bunched these together as I don't think any are controversial and it seems a waste of time to review each independently.
[libclc] Update deprecated method use in remangler
llvm::StringRef::startswith -> llvm::StringRef::starts_with
[libclc] Fix up improper use of ExitOnError
ExitOnError takes a llvm::Expected, but we were passing it the result of
llvm::parseIR - std::unique_ptrllvm::Module - which, even when null, is
not an error condition. Thus invalid IR input was silently being
accepted until it would segfault on accessing the module.
[libclc] Open remangler file system at PWD, not root
I don't think there's any reason to open it at root. Opening it at the
current working directory is more intuitive for developers using
relative paths for input/output files.
Previously "--input-ir foo.ll" would try and open "/foo.ll", which
depending on the system is likely a permissions error, or a missing
file, or even an unintended file.