[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd added a comment. @thakis, thanks for the heads up, I've disabled the test on Windows - I should've disabled it on Windows in the first place. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88859/new/ https://reviews.llvm.org/D88859

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

thakis added a comment. Looks like the test doesn't pass on Windows: http://45.33.8.238/win/27342/step_7.txt Please take a look, and revert if it takes a while to fix. Here's the output of the diff on that Win bot: thakis@thakis6-w MINGW64 /c/src/llvm-project (merge) $ diff

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG82f86ae01a54: APINotes: add APINotesYAMLCompiler (authored by compnerd). Changed prior to commit:

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd added a comment. Thanks for the reviews! I'll reformat the rest of the code before merging. Im going to be working on merging this downstream first before merging this upstream, so it will take a couple of days still. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Looks good to me now! Thanks for addressing my concerns. Comment at: clang/tools/apinotes-test/APINotesTest.cpp:15 + +static

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd updated this revision to Diff 302298. compnerd added a comment. rebase, clang-format, add comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88859/new/ https://reviews.llvm.org/D88859 Files:

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added inline comments. Comment at: clang/include/clang/APINotes/Types.h:25 +/// auditing. +enum class EnumExtensibilityKind { + None, compnerd wrote: > martong wrote: > > compnerd wrote: > > > martong wrote: > > > > compnerd wrote: > > > > > martong

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd added inline comments. Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:33 +namespace yaml { +template <> +struct ScalarEnumerationTraits { martong wrote: > Could you please clang-format the code? The `Lint: Pre-merge checks` are all > over the

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd added inline comments. Comment at: clang/include/clang/APINotes/Types.h:25 +/// auditing. +enum class EnumExtensibilityKind { + None, martong wrote: > compnerd wrote: > > martong wrote: > > > compnerd wrote: > > > > martong wrote: > > > > > This seems

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added inline comments. Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:33 +namespace yaml { +template <> +struct ScalarEnumerationTraits { Could you please clang-format the code? The `Lint: Pre-merge checks` are all over the code and makes the

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a comment. In D88859#2359399 , @compnerd wrote: >> I'd like to have maximum flexibility from the submitter by willing to change >> the details of the implementation > > I don't think that is a fair expectation - there are plenty of times

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd marked an inline comment as done. compnerd added a comment. > I'd like to have maximum flexibility from the submitter by willing to change > the details of the implementation I don't think that is a fair expectation - there are plenty of times where this is technical disagreement on

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

gribozavr2 added a comment. > Fair enough. But I don't think that Clang developers just copied the > implementation from GCC or from MSVC. No (nor we could copy the implementation for a number of reasons). However, we did have to be bug-for-bug compatible sometimes for compatibility.

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a comment. In D88859#2354377 , @gribozavr2 wrote: >> I am having a hard time to accept "this is how it is implemented in our >> fork" as a technical argument. Besides, I am not sure how could the Clang >> community benefit about being

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd marked an inline comment as done. compnerd added inline comments. Comment at: clang/include/clang/APINotes/Types.h:25 +/// auditing. +enum class EnumExtensibilityKind { + None, martong wrote: > This seems a bit redundant to `Attrs.td`. I'd prefer to

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd updated this revision to Diff 300815. compnerd added a comment. Add more testing coverage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88859/new/ https://reviews.llvm.org/D88859 Files:

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd updated this revision to Diff 300796. compnerd added a comment. Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88859/new/ https://reviews.llvm.org/D88859 Files:

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd marked 4 inline comments as done. compnerd added inline comments. Comment at: clang/lib/APINotes/APINotesYAMLCompiler.cpp:439 + static void enumeration(IO , EnumExtensibilityKind ) { +IO.enumCase(EEK, "none", EnumExtensibilityKind::None); +IO.enumCase(EEK,

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

gribozavr2 added a comment. > I am having a hard time to accept "this is how it is implemented in our fork" > as a technical argument. Besides, I am not sure how could the Clang community > benefit about being backward compatible with a specialized fork and thus > making superfluous

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd marked 3 inline comments as done. compnerd added a comment. In D88859#2339159 , @martong wrote: > I value that you guys have a prototype on a fork that is being used in > production. But, upstreaming and these reviews give us the chance to

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a reviewer: NoQ. martong added a subscriber: NoQ. martong added a comment. Herald added a subscriber: Charusso. Adding @NoQ as a reviewer, he might have some valuable comments, as he is very keen on this feature. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a comment. > as it is something which is actually in production already > ... > At the very least we need it for compatibility - this is already a shipping > feature. I value that you guys have a prototype on a fork that is being used in production. But, upstreaming and these

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd marked 3 inline comments as done. compnerd added a comment. Thanks for the feedback! Comment at: clang/include/clang/APINotes/Types.h:1 +//===-- Types.h - API Notes Data Types --*- C++ -*-===// +// martong wrote: > So we are

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

martong added a comment. Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James: http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html Comment at:

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88859/new/ https://reviews.llvm.org/D88859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

compnerd created this revision. compnerd added reviewers: gribozavr2, MForster. Herald added subscribers: jfb, mgorny. Herald added a project: clang. compnerd requested review of this revision. This adds the skeleton of the YAML Compiler for APINotes. This change only adds the YAML IO model for