[PATCH] D67031: [Clang][Bundler] Error reporting improvements

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev marked an inline comment as done.
sdmitriev added inline comments.


================
Comment at: clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp:41-44
-#include <memory>
-#include <string>
-#include <system_error>
-#include <vector>
----------------
Hahnfeld wrote:
> sdmitriev wrote:
> > Hahnfeld wrote:
> > > sdmitriev wrote:
> > > > Hahnfeld wrote:
> > > > > sdmitriev wrote:
> > > > > > Hahnfeld wrote:
> > > > > > > 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`?)
> > > > > > Right. Restored required system includes.
> > > > > `vector` is still removed which is definitely used. Are you 100% sure that `algorithm` and `cstddef` are not needed?
> > > > 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:)
> > > If you want to replace `vector` by `SmallVector`, this must be its own revision.
> > 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.
> 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 [[ https://llvm.org/docs/DeveloperPolicy.html#code-reviews | code reviews ]] mentions splitting patches and there's a whole section about [[ https://llvm.org/docs/DeveloperPolicy.html#incremental-development | Incremental Development ]].
I have reverted vector -> SmallVector change.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67031/new/

https://reviews.llvm.org/D67031