Page MenuHomePhabricator

[Clang][Bundler] Error reporting improvements
Needs ReviewPublic

Authored by sdmitriev on Fri, Aug 30, 8:01 PM.

Details

Summary
  • Changed FileHandler read/write methods to return llvm::Error
  • Using unified way of reporting errors
  • Removed trailing '.' from the error messages

Diff Detail

Event Timeline

sdmitriev created this revision.Fri, Aug 30, 8:01 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript

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)

  • std::unique_ptr
  • std::string
  • std::error_code
  • std::vector,

so I don't think these lines should be removed. Same goes for assert and probably also cstddef / cstdint (uint64_t?)

sdmitriev updated this revision to Diff 218224.Sat, Aug 31, 9:10 AM
sdmitriev retitled this revision from [Clang][Bundler] Error reporting improvements [NFC] to [Clang][Bundler] Error reporting improvements.

Addressed review comments.

sdmitriev marked an inline comment as done.Sat, Aug 31, 9:12 AM
sdmitriev added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
42–45

Right. Restored required system includes.

MaskRay added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
888

I think trailing full stop is uncommon in error messages.

sdmitriev marked an inline comment as done.Sat, Aug 31, 11:01 AM
sdmitriev added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
888

Maybe., but all error messages in this tool seem to be consistent in that sense)) Do you suggest removing trailing '.' from all error messages?

sdmitriev updated this revision to Diff 218250.Sat, Aug 31, 6:47 PM

Removed trailing '.' from error messages and added few additional changes for better error handling.

sdmitriev marked an inline comment as done.Wed, Sep 4, 11:04 AM

Any comments?

Hahnfeld added inline comments.Wed, Sep 4, 11:13 AM
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?

sdmitriev marked an inline comment as done.Wed, Sep 4, 11:21 AM
sdmitriev added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
42–45

I have replaced all uses of vector with SmallVector.

Are you 100% sure that algorithm and cstddef are not needed?

Ok, I will add algorithm and cstddef back:)

sdmitriev updated this revision to Diff 218755.Wed, Sep 4, 11:25 AM

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.

sdmitriev marked an inline comment as done.Wed, Sep 4, 12:24 PM
sdmitriev added inline comments.
clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp
42–45

Must? Can you show any document/link explaining this?
I have no problem with doing that in a separate commit, not a big deal, just want to see why it has to be done that way.

Hahnfeld added inline comments.Wed, Sep 4, 12:32 PM
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.

sdmitriev updated this revision to Diff 218776.Wed, Sep 4, 1:11 PM
sdmitriev edited the summary of this revision. (Show Details)
sdmitriev marked an inline comment as done.
sdmitriev added inline comments.
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?

sdmitriev added a reviewer: Hahnfeld.

Rebase