[libcxx-commits] [PATCH] D99615: [libc++] Improve generate_feature_test_macro_components.py.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 7 inline comments as done.
Mordante added a comment.

Thanks for the reviews, will commit an update shortly.



================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:33
+# ================  ============================================================
+# Field             Description
+# ================  ============================================================
----------------
Quuxplusone wrote:
> @Mordante: I propose that you lose the `s` in `libcxx_guards` and `test_suite_guards`: just do `libcxx_guard` and `test_suite_guard`. These are single strings, not arrays of strings; and shorter is better anyway.
> (Btw, I always interpreted `depends` as the present-tense singular verb, as in "depends on," not as an abbreviation of "dependencies.")
> 
> I would not object if someone dug up a precedent for spelling it `libcxx-guard`.
I used the naming Louis proposed in D99290 but I don't mind to make it the name singular. 


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:36-39
+# values            A list of dictionaries. One dictionary contains a C++
+#                   version and the value of the feature-test macro for that
+#                   C++ version. (Note: currently there's no proper support
+#                   when 2 papers in a C++ version modify the value.)
----------------
Quuxplusone wrote:
> 
I like your wording, but I dislike abbreviations in a text so will replace them.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:52
+#                   It shall not depend on a macro defined in
+#                   `include/__config`.
+# libcxx_guards      An optional string field. When this field is provided
----------------
ldionne wrote:
> Quuxplusone wrote:
> > (A) why not?
> > (B) I think you can cut lines 44-50, and just let lines 51-52 do the talking
> > (C) I think you need to say explicitly that `test_suite_guards` is used in the generated tests (but not in the libc++ headers); whereas `libcxx_guards` is used in libc++'s `<version>` header (but not in the tests). That's the main point of these fields AIUI.
> > (A) why not?
> 
> Because the test suite is supposed to work even on non-libc++ standard libraries.
> (B) I think you can cut lines 44-50, and just let lines 51-52 do the talking

I'll look at removing some fluff of 44-50, but I think it's good to state a more explicitly what's expected for this field.

> (C) I think you need to say explicitly that `test_suite_guards` is used in the generated tests (but not in the libc++ headers); whereas `libcxx_guards` is used in libc++'s `<version>` header (but not in the tests). That's the main point of these fields AIUI.

Good point.



================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:57
+#                   * when available, a macro defined in `include/__config`,
+#                   * otherwise the same as `test_suite_guards`.
+# unimplemented     An optional Boolean field with the value `True`. This field
----------------
Quuxplusone wrote:
> Is this "otherwise" codepath actually hit at the moment? Could we eliminate this whole codepath by duplicating a little bit of data in the table?
Not entirely sure what you mean. This means you need to write to write the same value for `ibcxx_guards` and `test_suite_guards`. For example see line 68-69 with both depend on the CMake defined macro `"!defined(_LIBCPP_HAS_NO_THREADS)"`


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:670
 assert feature_test_macros == sorted(feature_test_macros, key=lambda tc: tc["name"])
 assert all(tc["headers"] == sorted(tc["headers"]) for tc in feature_test_macros)
 
----------------
Quuxplusone wrote:
> Let's throw in another couple asserts here like
> ```
> assert all(("libcxx_guards" in tc) == ("test_suite_guards" in tc) for tc in feature_test_macros)
> assert all(key in ["name", "values", "headers", "libcxx_guards", "test_suite_guards", "unimplemented"] for key in tc.keys() for tc in feature_test_macros)
> ```
> The first one allows you to remove the assert from line 755.
> The second one guards against the fact that it's way easier to carelessly misspell `libcxx_guards` than it was to misspell `depends`.
> (I don't actually understand the assert on line 759.)
> Let's throw in another couple asserts here like
> ```
> assert all(key in ["name", "values", "headers", "libcxx_guards", "test_suite_guards", "unimplemented"] for key in tc.keys() for tc in feature_test_macros)
> ```

This one doesn't work, I adjusted it to a working version. My Python is a bit rusty, so please have a look at it.

> The first one allows you to remove the assert from line 755.
I like this.

> (I don't actually understand the assert on line 759.)
Me neither, so I'll just keep it.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99615/new/

https://reviews.llvm.org/D99615