[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

MyDeveloperDay added a comment. The issue that caused this issue was fix by the fix in https://reviews.llvm.org/rGa92cf5a5a0cd01145f8db2ae09334a8b43a1271b , from the clang-format perspective I don't really feel we need to revisit bringing in these libraries. I think I proved that moving

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dblaikie added a comment. This is a bit out of left field, but these comments reminded me of something - a long time ago I was working on fixing some Clang layering to potentially use explicit modules more in our internal build (& then hopefully in the upstream/external build) and one major

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

HazardyKnusperkeks abandoned this revision. HazardyKnusperkeks added a comment. In D90121#3065063 , @dmikis wrote: > Since we used heavily patched version of clang-format (including this patch) > that problem didn't bugged us and I kinda moved on to

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. > I took a look and in its simplest form moving TextDiagnosticPrinter.cpp to > lib/Basic but also requires DiagnosticRender.cpp and TextDiagnostic.cpp to > also move to lib/Basic too, > > But DiagnosticRender.cpp has a dependency on lib/Edit so clang-format now > needs

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

rgets I perform a "ninja clean" so that hopefully the build that follows shows me how many targets I needed 1. Using the current tip of master, the number of targets to build clang-format sits at 608 2. If I apply this FrontEnd patch D90121: clang-format: Add a consumer to diagnostics engi

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

thakis added a comment. Thanks for the test! I think moving TextDiagnosticsPrinter to Basic is much better than making Format depend on Frontend. If you just want to fix your crash and be done with it, then doing that (and adding your test) is fine. I think the best fix would be to change

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

MyDeveloperDay added a comment. @dmikis just for history sake I introduce the frontEnd when doing D68554: [clang-format] Proposal for clang-format to give compiler style warnings , D69854: [clang-format] [RELAND] Remove the dependency on frontend

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @MyDeveloperDay That change introduced a regression: clang-format crashes instead of reporting errors in certain situations. See my comment with a test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

MyDeveloperDay added a comment. @thakis didn't you and I go through this process of removing frontEnd once before? do we really want to add this back in for all the reasons you said before? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @thakis I've got test working, here's the code: // RUN: chmod +w %t.dir || true // RUN: rm -rf %t.dir // RUN: mkdir %t.dir // RUN: cp %s %t.dir/read-only.cpp // RUN: chmod -w %t.dir // RUN: clang-format -style=LLVM -i %t.dir/read-only.cpp || test $? == 1

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

thakis added a comment. Is it possible to add an automated test for this in clang/test/Format/... using chmod -w in a RUN line? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/ https://reviews.llvm.org/D90121

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @thakis We've stumbled upon this problem when tried to format in-place a read-only file. `clang-format` tries to report this problem via `DiagnosticEngine`, and that fails on assert

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

thakis added a comment. Can you expand a bit on what exactly is going wrong? (As part of that, please include a test case that shows the crash this is about.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @thakis There's a simple option of moving `TextDiagnosticPrinter` into `libBasic` thus eliminating neccessety to depend on `libFrontend`. How does that sound? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

krasimir added a comment. In D90121#2361985 , @thakis wrote: > This doe snot lgtm. clang-format should not depend on clangFrontend, see the > lengthy discussion in https://reviews.llvm.org/D68554 . Let's try to find > another fix here (and include a

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

thakis reopened this revision. thakis added a comment. This revision is now accepted and ready to land. This doe snot lgtm. clang-format should not depend on clangFrontend, see the lengthy discussion in https://reviews.llvm.org/D68554 . Let's try to find another fix here (and include a test for

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @krasimir Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/ https://reviews.llvm.org/D90121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdf00267f1fdb: clang-format: Add a consumer to diagnostics engine (authored by krasimir). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

krasimir added a comment. In D90121#2358730 , @dmikis wrote: > @krasimir What would our next steps be to get this change into LLVM source > tree? JIC, I don't have commit rights :) I'll commit this for you. If you're planning to commit regularly, and

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. @krasimir What would our next steps be to get this change into LLVM source tree? JIC, I don't have commit rights :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90121/new/ https://reviews.llvm.org/D90121

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

krasimir accepted this revision. krasimir added a comment. This revision is now accepted and ready to land. Thank you! Your fix looks good for clang-format. I'm also not familiar with `DiagnosticsEngine`. It would be nice to follow-up with folks on that on either removing the defaulted ctor

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dmikis added a comment. > If this usage leads to crashes, isn't the issue in DiagnosticsEngine itself? I don't know design intent behind `DiagnosticsEngine`. As far as I can understand from source code it doesn't try to create any consumers by itself yet requires one to be provided (there're

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

krasimir added a comment. This looks reasonable. I'm curious about DiagnosticsEngine: previously we were using the defaults for `client` and `ShouldOwnClient` ctor params (`nullptr` and `true`). If this usage leads to crashes, isn't the issue in `DiagnosticsEngine` itself? Repository: rG