whisperity added a comment. What is the motivation behind the change? Can we be sure that these classes that were moved out weren't details to the context itself, and are understandable, usable, and can be relied upon without the context object?
---- In D113848#3130542 <https://reviews.llvm.org/D113848#3130542>, @carlosgalvezp wrote: > I agree with the comments, but I didn't want to touch any code other than > moving things around, since it's hard to see the changes in the diff > otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm > happy to fix in a separate patch if that's OK? I think you can fix it now, LLVM style doesn't indent stuff that's within a namespace, so it won't change the meaningful part of the diff visually. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyContext.cpp:40-41 +#include <vector> +using namespace clang; +using namespace tidy; + ---------------- Eugene.Zelenko wrote: > Shouldn't namespaces be used instead? In general, you'd also want to make it more explicit by saying ``` using namespace clang; using namespace clang::tidy; ``` and not rely on precisely one `tidy` being visible from the previous set of available declarations. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyError.h:35-36 + +} // end namespace tidy +} // end namespace clang + ---------------- Is this formatted properly? I thought the appropriate closing comment is `// namespace blah`, without the `end`. 😲 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113848/new/ https://reviews.llvm.org/D113848 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
