- Changed FileHandler read/write methods to return llvm::Error
- Using unified way of reporting errors
- Removed trailing '.' from the error messages
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This changes error messages, so I'd say it's not NFC.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | The code still uses (in the order of marked includes)
so I don't think these lines should be removed. Same goes for assert and probably also cstddef / cstdint (uint64_t?) |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | Right. Restored required system includes. |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
898 | I think trailing full stop is uncommon in error messages. |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
898 | Maybe., but all error messages in this tool seem to be consistent in that sense)) Do you suggest removing trailing '.' from all error messages? |
Removed trailing '.' from error messages and added few additional changes for better error handling.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | vector is still removed which is definitely used. Are you 100% sure that algorithm and cstddef are not needed? |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | I have replaced all uses of vector with SmallVector.
Ok, I will add algorithm and cstddef back:) |
Also, there should be a summary of the changes in here.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | If you want to replace vector by SmallVector, this must be its own revision. |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | Must? Can you show any document/link explaining this? |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | This is common practice for reviews: Only one change per patch and split into smaller patches if possible and logically appropriate. I can't quote a written requirement, but the Developer Policy section about code reviews mentions splitting patches and there's a whole section about Incremental Development. |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
42–45 | I have reverted vector -> SmallVector change. |
I think I have addressed all comments posted so far. Do you have more notes/comments/suggestions?
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
132–136 | Do we really need an inner Optional here? |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
132–136 | The idea was to return StringRef with bundle name when we have still have bundles and None when there are no more bundles in the file (BTW comment has to be updated to describe this). But I can restore the old convention where empty bundle name means 'no more bundles' in the file. In terms of functionality that would be the same, though use of Optional<...> makes intentions more explicit)) |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
132–136 | The problem here is that Expected already handles the state and we have inner Optional for the same reason. Can we reuse Expected to indicate that there are no more bundles? |
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp | ||
---|---|---|
132–136 | Well, Expected encodes two possible states (either Error or Value), but we need to encode three states - Error, Value or None. I do not have ideas how to do it without adding inner Optional. If you have, please share. |
The code still uses (in the order of marked includes)
so I don't think these lines should be removed. Same goes for assert and probably also cstddef / cstdint (uint64_t?)