[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3426189 , @bjope wrote: > In D121233#3426155 , @sammccall > wrote: > >> In D121233#3425793 , @bjope wrote: >> >>> Have had problems

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

bjope added a comment. In D121233#3426155 , @sammccall wrote: > In D121233#3425793 , @bjope wrote: > >> Have had problems with failing stage 2 builds since this patch: > > Should be fixed in 72ae6cc3a689869dcc15cf

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3425793 , @bjope wrote: > Have had problems with failing stage 2 builds since this patch: Should be fixed in 72ae6cc3a689869dcc15cfa8d0abcdd35a35a484

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3425793 , @bjope wrote: > Have had problems with failing stage 2 builds since this patch: > > FAILED: > tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o > > /repo/

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

bjope added a comment. Have had problems with failing stage 2 builds since this patch: FAILED: tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o /repo//install/stage2/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MAC

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

thakis added a comment. In D121233#3384567 , @sammccall wrote: > In D121233#3384495 , @thakis wrote: > >> Is there a reason why this can't be part of clang-tools-extra/test > > This was discussed pretty extensivel

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3384495 , @thakis wrote: > Is there a reason why this can't be part of clang-tools-extra/test This was discussed pretty extensively in the review above. Other projects across LLVM use proj/{lib,include,unittest,test}

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

thakis added a comment. Is there a reason why this can't be part of clang-tools-extra/test and check-clang-tools? Why is this in a directory called "pseudo" instead of "clang-pseudo"? This makes everything fairly self-inconsistent (`pseudo/tool` vs `clang-pseudo` in there, but then also `check

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. Ah I see what happened - there was one incorrect (and unneeded) dependency left behind, which caused tests to fail if you *didn't* have clang-tools-extra enabled. Relanding... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

Re: [PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

Sorry, this broke the world and I wasn't watching it carefully enough. Reverted in 049f4e4eab19c6e468e029232e94ca71245b0f56, trying to work out why the tests passed locally. On Wed, Mar 16, 2022 at 12:48 AM Ron Lieberman via Phabricator < revi...@reviews.llvm.org> wrote: > ronlieb added a comment

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

ronlieb added a comment. Hi Sam i think this breaks our amdgpu buildbot https://lab.llvm.org/buildbot/#/builders/193/builds/8513 any suggestions? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121233/new/ https://reviews.llvm.org/D121233 _

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb97856c4cfe7: [pseudo] Move pseudoparser from clang to clang-tools-extra (authored by sammccall). Changed prior to commit: https://reviews.llvm.or

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall updated this revision to Diff 415619. sammccall marked 4 inline comments as done. sammccall added a comment. Rebase and restore conventional test/Unit suite Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121233/new/ https://reviews.llvm.or

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

aaron.ballman added a comment. In D121233#3371422 , @sammccall wrote: > Thanks Aaron, and my apologies for being easily frustrated. No worries at all! I always appreciate our discussions, even when we don't initially agree. > In D121233#3370992

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. Thanks Aaron, and my apologies for being easily frustrated. In D121233#3370992 , @aaron.ballman wrote: > 2. The layout of clang-tools-extra in terms of test directories is > problematic and it'd sure be nice if someday someon

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

aaron.ballman accepted this revision. aaron.ballman added a comment. In D121233#3370821 , @sammccall wrote: > In D121233#3370177 , @aaron.ballman > wrote: > >> I agreed based on the understanding that the new cod

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3370177 , @aaron.ballman wrote: > I agreed based on the understanding that the new code would follow the > existing conventions, so perhaps the agreement was a bit premature. But, I'm > honestly pretty surprised at

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

aaron.ballman added a comment. In D121233#3370110 , @sammccall wrote: > I understand where you're coming from. But I think agreeing to move the code > was premature if it means either: > > - following all the precedents in clang-tools-extra, or > - takin

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. I understand where you're coming from. But I think agreeing to move the code was premature if it means either: - following all the precedents in clang-tools-extra, or - taking on the political burden of getting these changed for all projects. Based on previous interac

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

aaron.ballman added a comment. In D121233#3369736 , @sammccall wrote: > In D121233#3369657 , @aaron.ballman > wrote: > >> Thank you for working on this! I have a few thoughts on the renaming, but >> otherwise st

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. In D121233#3369657 , @aaron.ballman wrote: > Thank you for working on this! I have a few thoughts on the renaming, but > otherwise strongly support the direction here. > >> clang/lib/Tooling/Syntax/Pseudo/* => clang-

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

aaron.ballman added a comment. Thank you for working on this! I have a few thoughts on the renaming, but otherwise strongly support the direction here. > clang/lib/Tooling/Syntax/Pseudo/* => clang-tools-extra/pseudo/lib/* The usual naming conventions in clang-tools-extra is to use the

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

hokein accepted this revision. hokein added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/pseudo/CMakeLists.txt:1 +include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include) +add_subdirectory(lib) sammccall

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added inline comments. Comment at: clang-tools-extra/pseudo/CMakeLists.txt:1 +include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include) +add_subdirectory(lib) hokein wrote: > Why not use `CMAKE_CURRENT_SOURCE_DIR`? It seems a more natural fit. I

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

hokein added a comment. Thanks, the movement looks roughly good. We should be aware of that (when landing this patch): - the upstream llvm gn build files need to be adjust (IIUC, there is no expectation from us to update the gn files) - we'd better prepare a patch for the internal integration (

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall added a comment. Herald added a subscriber: ormris. @hokein I talked about making some headers private, but realized that change really is unrelated, and would be confusing to mix in here. This is awkward to review. I think the main things to check: - whether the intended renames (see

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

sammccall created this revision. sammccall added reviewers: hokein, aaron.ballman. Herald added a subscriber: mgorny. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, alextsao1999. Herald added projects: clang, clang-tools-extra. This