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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
