[libc++] Adds a new feature-test macro generator class. #90889

mordante · 2024-05-02T18:52:21Z

The new feature-test macro generator uses a JSON file as input. Separating the
code from the data allows for testing the code. The generator has several tests.

This JSON format is based on the new format proposed in #88630

At the moment the FTM script has the existing code and an unused new
generator. Followup patches will complete the generator and convert the existing
Python dict to the new JSON format. Since that conversion is a manual job and
quite a bit of work the transition path has some unused code for some time.

This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing. This is related to the RFC #89499

llvmbot · 2024-05-02T18:53:03Z

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing.

This is related to the RFC #89499

Full diff: https://github.com/llvm/llvm-project/pull/90889.diff

3 Files Affected:

(added) libcxx/test/libcxx/feature_test_macro_csv.sh.py (+35)
(added) libcxx/utils/data/feature_test_macros/test_data.json (+136)
(modified) libcxx/utils/generate_feature_test_macro_components.py (+33)

diff --git a/libcxx/test/libcxx/feature_test_macro_csv.sh.py b/libcxx/test/libcxx/feature_test_macro_csv.sh.py
new file mode 100644
index 00000000000000..2d80bfad63c859
--- /dev/null
+++ b/libcxx/test/libcxx/feature_test_macro_csv.sh.py
@@ -0,0 +1,35 @@
+# RUN: %{python} %s %{libcxx-dir}/utils %{libcxx-dir}/utils/data/feature_test_macros/test_data.json
+
+import sys
+import json
+
+sys.path.append(sys.argv[1])
+from generate_feature_test_macro_components import get_table
+
+
+data = json.load(open(f"{sys.argv[2]}"))
+table = get_table(data)
+
+expected = {
+    "__cpp_lib_any": {
+        "c++17": "201606L",
+        "c++20": "201606L",
+        "c++23": "201606L",
+        "c++26": "201606L",
+    },
+    "__cpp_lib_barrier": {"c++20": "201907L", "c++23": "201907L", "c++26": "201907L"},
+    "__cpp_lib_format": {
+        "c++20": "",
+        "c++23": "",
+        "c++26": "",
+    },
+    "__cpp_lib_variant": {
+        "c++17": "202102L",
+        "c++20": "202102L",
+        "c++23": "202102L",
+        "c++26": "202102L",
+    },
+}
+
+
+assert table == expected, f"expected\n{expected}\n\nresult\n{table}"
diff --git a/libcxx/utils/data/feature_test_macros/test_data.json b/libcxx/utils/data/feature_test_macros/test_data.json
new file mode 100644
index 00000000000000..5a98fba6403c09
--- /dev/null
+++ b/libcxx/utils/data/feature_test_macros/test_data.json
@@ -0,0 +1,136 @@
+[
+  {
+    "name": "__cpp_lib_any",
+    "values": {
+      "c++17": {
+        "201606": [
+          {
+            "implemented": true
+          }
+        ]
+      }
+    },
+    "headers": [
+      "any"
+    ]
+  },
+  {
+    "name": "__cpp_lib_barrier",
+    "values": {
+      "c++20": {
+        "201907": [
+          {
+            "implemented": true
+          }
+        ]
+      }
+    },
+    "headers": [
+      "barrier"
+    ],
+    "test_suite_guard":
+        "!defined(_LIBCPP_HAS_NO_THREADS) && (!defined(_LIBCPP_VERSION) || _LIBCPP_AVAILABILITY_HAS_SYNC)",
+    "libcxx_guard": "!defined(_LIBCPP_HAS_NO_THREADS) && _LIBCPP_AVAILABILITY_HAS_SYNC"
+  },
+  {
+    "name": "__cpp_lib_format",
+    "values": {
+      "c++20": {
+        "201907": [
+          {
+            "number": "P0645R10",
+            "title": "Text Formatting",
+            "implemented": true
+          },
+          {
+            "number": "P1361R2",
+            "title": "Integration of chrono with text formatting",
+            "implemented": false
+          }
+        ],
+        "202106": [
+          {
+            "number": "P2216R3",
+            "title": "std::format improvements",
+            "implemented": true
+          }
+        ],
+        "202110": [
+          {
+            "number": "P2372R3",
+            "title": "Fixing locale handling in chrono formatters",
+            "implemented": false
+          },
+          {
+            "number": "P2418R2",
+            "title": "FAdd support for std::generator-like types to std::format",
+            "implemented": true
+          }
+        ]
+      },
+      "c++23": {
+        "202207": [
+          {
+            "number": "P2419R2",
+            "title": "Clarify handling of encodings in localized formatting of chrono types",
+            "implemented": false
+          }
+        ]
+      },
+      "c++26": {
+        "202306": [
+          {
+            "number": "P2637R3",
+            "title": "Member Visit",
+            "implemented": true
+          }
+        ],
+        "202311": [
+          {
+            "number": "P2918R2",
+            "title": "Runtime format strings II",
+            "implemented": true
+          }
+        ]
+      }
+    },
+    "headers": [
+      "format"
+    ]
+  },
+  {
+    "name": "__cpp_lib_variant",
+    "values": {
+      "c++17": {
+        "202102": [
+          {
+            "number": "",
+            "title": "``std::visit`` for classes derived from ``std::variant``",
+            "implemented": true
+          }
+        ]
+      },
+      "c++20": {
+        "202106": [
+          {
+            "number": "",
+            "title": "Fully constexpr ``std::variant``",
+            "implemented": false
+          }
+        ]
+      },
+      "c++26": {
+        "202306": [
+          {
+            "number": "",
+            "title": "Member visit",
+            "implemented": true
+          }
+        ]
+      }
+    },
+    "headers": [
+      "variant"
+    ]
+  }
+]
diff --git a/libcxx/utils/generate_feature_test_macro_components.py b/libcxx/utils/generate_feature_test_macro_components.py
index f2b8d55c0e11b0..04a8a8786b1a35 100755
--- a/libcxx/utils/generate_feature_test_macro_components.py
+++ b/libcxx/utils/generate_feature_test_macro_components.py
@@ -3,6 +3,7 @@
 import os
 from builtins import range
 from functools import reduce
+import json
 
 
 def get_libcxx_paths():
@@ -1857,6 +1858,38 @@ def produce_docs():
         f.write(doc_str)
 
 
+def get_table(data):
+    result = dict()
+    for feature in data:
+        last = None
+        entry = dict()
+        implemented = True
+        for std in get_std_dialects():
+            if std not in feature["values"].keys():
+                if last == None:
+                    continue
+                else:
+                    entry[std] = last
+            else:
+                if last == None:
+                    last = ""
+                if implemented:
+                    for value in feature["values"][std]:
+                        for paper in list(feature["values"][std][value]):
+                            if not paper["implemented"]:
+                                implemented = False
+                                break
+                        if implemented:
+                            last = f"{value}L"
+                        else:
+                            break
+
+                entry[std] = last
+        result[feature["name"]] = entry
+
+    return result
+
+
 def main():
     produce_version_header()
     produce_tests()

EricWF

Looks good to me.

ldionne

The approach LGTM but I'd like to see this again!

libcxx/test/libcxx/feature_test_macro_csv.sh.py

ldionne · 2024-05-07T16:26:33Z

libcxx/test/libcxx/feature_test_macro_csv.sh.py

@@ -0,0 +1,35 @@
+# RUN: %{python} %s %{libcxx-dir}/utils %{libcxx-dir}/utils/data/feature_test_macros/test_data.json
+
+import sys


Maybe rename to libcxx/test/libcxx/feature_test_macros/get_table.sh.py? We'll have other tests in that subdirectory later.

Based on other review comments there no longer is a get_table function. I removed this file and added the new tests in the suggested s

ldionne · 2024-05-07T16:29:14Z

libcxx/utils/generate_feature_test_macro_components.py

@@ -1857,6 +1858,38 @@ def produce_docs():
        f.write(doc_str)


+def get_table(data):


Can you document the format of the data taken by this function? Also, the script assumes a lot of global state right now (like get_std_dialects()), it would be nice to improve on this as we're basically rewriting this. Can't we actually infer the standard dialects from the json data we're reading?

We could potentially have a Python class that represents this JSON data and provides a few useful attributes like .dialects and more, and then we could query the things we implement with something like

for std in table.dialects: for ftm in table.entries: if ftm.implemented: # print information for this FTM in this dialect

I disagree with the "a lot of global state". It only requires get_std_dialects().

I've looked at your Python class suggestion and extracting the C++ dialects from the data. This indeed seems to work and we can indeed extract the dialects from the data.

I've updated the documentation and added more tests; the next version of the patch no longer is considered RFC.

ldionne

This looks nice! I have some comments but this should be good to go then.

ldionne · 2024-05-23T15:48:32Z

libcxx/utils/data/feature_test_macros/test_data.json

@@ -0,0 +1,151 @@
+[


I would suggest moving this file under test since it contains test data.

ldionne · 2024-05-23T15:51:35Z

libcxx/utils/generate_feature_test_macro_components.py

+    return sorted(dialects)
+
+
+def get_dialect_versions(data, std_dialects, use_implemented_status):


Suggested change

def get_dialect_versions(data, std_dialects, use_implemented_status):

def get_dialect_versions(data, std_dialects: List[str], use_implemented_status: bool) -> Map[str, Map[str, int]]:

ldionne · 2024-05-23T15:52:14Z

libcxx/utils/generate_feature_test_macro_components.py

+        """Initializes the class with the JSON data in the file 'filename'."""
+        self.__data = json.load(open(filename))
+
+    def get_std_dialects(self):


Suggested change

def get_std_dialects(self):

@functools.cached_property

def std_dialects(self) -> List[str]:

This seems a bit more Pythonic to me. And then the implementation can be just return get_std_dialects(self.__data).

ldionne · 2024-05-23T15:55:14Z

libcxx/utils/generate_feature_test_macro_components.py

@@ -1857,6 +1858,215 @@ def produce_docs():
        f.write(doc_str)


+def get_std_dialects(data):


I think inlining this one inside the class will help readability.

ldionne · 2024-05-23T16:05:47Z

libcxx/utils/generate_feature_test_macro_components.py

+
+        return self.__std_dialects
+
+    def get_std_dialect_versions(self):


We could instead provide the following interface:

def get_standard_ftms(self, dialect: str) -> List[Tuple[str, int]]: # returns the list of ('__cpp_lib_foo', 2017xxx) FTMs in the Standard def get_implemented_ftms(self, dialect: str) -> List[Tuple[str, int]]: # returns the list of ('__cpp_lib_foo', 2017xxx) FTMs implemented by libc++

ldionne · 2024-05-23T16:08:51Z

libcxx/utils/generate_feature_test_macro_components.py

+    # These values are used internally multiple times. They are lazily loaded
+    # and cached. Values that are expected to be used once are not cached.
+    __std_dialects = None
+    __std_dialect_versions = None
+    __dialect_versions = None


Suggested change

# These values are used internally multiple times. They are lazily loaded

# and cached. Values that are expected to be used once are not cached.

__std_dialects = None

__std_dialect_versions = None

__dialect_versions = None

ldionne · 2024-05-23T16:10:15Z

libcxx/utils/generate_feature_test_macro_components.py

+        assert len(keys) > 0, "'values' is empty"
+        dialects |= keys
+
+    return sorted(dialects)


Suggested change

return sorted(dialects)

return sorted(list(dialects))

Because the attribute is going to return a List[str].

ldionne · 2024-05-23T16:11:19Z

libcxx/test/libcxx/feature_test_macro/get_dialect_versions.sh.py

@@ -0,0 +1,53 @@
+# RUN: %{python} %s %{libcxx-dir}/utils %{libcxx-dir}/utils/data/feature_test_macros/test_data.json


Nit, but I think these RUN lines should come after the license.

ldionne · 2024-05-23T16:12:25Z

libcxx/test/libcxx/feature_test_macro/get_dialect_versions.sh.py

+    assert output == expected, f"expected\n{expected}\n\noutput\n{output}"
+
+
+fmt = feature_test_macros(sys.argv[2])


Suggested change

fmt = feature_test_macros(sys.argv[2])

ftm = feature_test_macros(sys.argv[2])

ldionne · 2024-05-23T16:14:00Z

libcxx/test/libcxx/feature_test_macro/get_dialect_versions.sh.py

+from generate_feature_test_macro_components import feature_test_macros
+
+
+def test(output, expected):


Let's use Python's unittest module instead.

[RFC][libc++] Testing feature test macro script.

f5c591c

This is a proof-of-concept how we can test the script. Instead of storing the data in the script it's stored in a JSON file so a different file can be used for testing. This is related to the RFC #89499

mordante requested a review from a team as a code owner May 2, 2024 18:52

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 2, 2024

mordante requested a review from EricWF May 2, 2024 18:53

EricWF approved these changes May 3, 2024

View reviewed changes

ldionne reviewed May 7, 2024

View reviewed changes

mordante changed the title ~~[RFC][libc++] Testing feature test macro script.~~ [libc++] Adds a new feature-test macro generator class. May 9, 2024

Addresses review comments.

b820528

ldionne requested changes May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Adds a new feature-test macro generator class. #90889

[libc++] Adds a new feature-test macro generator class. #90889

mordante commented May 2, 2024 •

edited

llvmbot commented May 2, 2024

EricWF left a comment

ldionne left a comment

ldionne May 7, 2024

mordante May 9, 2024

ldionne May 7, 2024

mordante May 9, 2024

ldionne left a comment

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

ldionne May 23, 2024

		@@ -0,0 +1,35 @@
		# RUN: %{python} %s %{libcxx-dir}/utils %{libcxx-dir}/utils/data/feature_test_macros/test_data.json

		import sys

		@@ -1857,6 +1858,38 @@ def produce_docs():
		f.write(doc_str)


		def get_table(data):

		return sorted(dialects)


		def get_dialect_versions(data, std_dialects, use_implemented_status):

	def get_std_dialects(self):
	@functools.cached_property
	def std_dialects(self) -> List[str]:

		@@ -1857,6 +1858,215 @@ def produce_docs():
		f.write(doc_str)


		def get_std_dialects(data):


		return self.__std_dialects

		def get_std_dialect_versions(self):

		@@ -0,0 +1,53 @@
		# RUN: %{python} %s %{libcxx-dir}/utils %{libcxx-dir}/utils/data/feature_test_macros/test_data.json

		assert output == expected, f"expected\n{expected}\n\noutput\n{output}"


		fmt = feature_test_macros(sys.argv[2])

	fmt = feature_test_macros(sys.argv[2])
	ftm = feature_test_macros(sys.argv[2])

		from generate_feature_test_macro_components import feature_test_macros


		def test(output, expected):

[libc++] Adds a new feature-test macro generator class. #90889

Are you sure you want to change the base?

[libc++] Adds a new feature-test macro generator class. #90889

Conversation

mordante commented May 2, 2024 • edited

llvmbot commented May 2, 2024

EricWF left a comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mordante commented May 2, 2024 •

edited