MSVC C++20 Support #126

sean-parent · 2024-11-07T05:34:41Z

Added Support for MSVC C++20.
Fixed warnings at \W4
Improved cmake presets support for VSCode.
New libraries:

macro_utilities.hpp
- ADOBE_STRINGIZE(x)
- ADOBE_LINE_STRING()
- ADOBE_MESSAGE(message)
exception.hpp
- terminate(message)
append.hpp
- append(container, range)
unique.hpp
- sort_unique(container)

… Compiling with MSVC at \W4.

sean-parent · 2024-11-07T06:26:32Z

test/adam_smoke/CMakeLists.txt

+    target_compile_options (adam_smoke_test PRIVATE "/Zc:__cplusplus")
+    target_compile_options (adam_smoke_test PRIVATE "/W4")
+    target_compile_options (adam_smoke_test PRIVATE "/WX")
+endif()


@dabrahams @camio - There must be a better way to manage compiler settings.
I want the library to require the first 3 options, and the cilents (test cases) to add the second 2 options.

This is how I think about it: For portable libraries, the consumer of the library determines the compiler+flags combination for building the library. When the library writer dictates flags, the library gets tied to a snapshot of today's compilers and options making it unportable. The library writer can, however, require certain compiler capabilities.

The requirement underlying the desire for -DNOMINMAX is a WinDef.h that doesn't define min/max macros. I'd enforce this in CMake like this

if(WIN32) check_cxx_symbol_exists(min WinDef.h ASL_WINDEF_DEFINES_MIN_MAX) if(ASL_WINDEF_DEFINES_MIN_MAX) message(FATAL_ERROR "WinDef.h defining `min` and `max` is unsupported by this library. If using MSVC, adding -DNOMINMAX to your compiler flags will address this issue.") endif() endif()

The same goes for the other two options you want to require. The idea is to check for the functionality you require using something like check_cxx_source_compiles and providing helpful messages if they fail.

The last two options that you want for a single file are only useful for developers of this library, but problematic for users and integrators (especially when those flags prevent this library from working with a new compiler version).

If applying these flags to the entire repository isn't feasible, I'd suggest adding a CMake option (defaulting to FALSE) for enabling these flags. Then your code would look something like this:

if (ASL_FORCE_WARNINGS AND "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") target_compile_options (adam_smoke_test PRIVATE "/W4") target_compile_options (adam_smoke_test PRIVATE "/WX") endif()

The next question to ask is how are users going to know which flags they need to pass to the various compilers they use. This is what presets are for. With the right preset a user should be able to use cmake --workflow --preset msvc-release to get everything built and tested.

If by

I want the library to require the first 3 options and the cilents (test cases) to add the second 2 options.

You actually mean you want the library to be built with the first 3 options and its clients to have the 2nd 2 options imposed on them automatically due to the fact that they link the library, then you want

target_compile_options(the_library PRIVATE "/Zc:__cplusplus" PUBLIC "/W4" "/WX")

The last two become usage requirements as well as requirements for building the library. To make them pure usage requirements, you'd s/PUBLIC/INTERFACE/.

@camio's statement that:

When the library writer dictates flags, the library gets tied to a snapshot of today's compilers and options making it unportable

Could be interpreted two ways: 1, you should try to use higher-level constructs than the flags for specific compilers whenever possible, or 2, you should never employ usage requirements. I agree with the first but would disagree with the second. I'm not sure what these flags do or whether higher-level constructs exist for them, though.

I updated it to use PRIVATE/PUBLIC (more) appropriately.

sean-parent · 2024-11-08T06:21:32Z

adobe/manip.hpp

+    However, if it is not included here, the build fails for C++17 under MSVC only. I have not been
+    able to determine why this is the case.
+*/
+#include <adobe/serializable.hpp>


@dabrahams - I was able to significantly reduce the problem to the pointer where I determined that adobe/serializable.hpp had to be included before manip.hpp, but there does not appear to be any dependency on serializable.hpp - circular or not. In fact, in my reduced case, not include manip.hpp at all also worked. If it was more than one configuration of one compiler, I'd reduce it more.

Wihtout this line the following fails to compiler:

#include <adobe/array.hpp> #include <adobe/manip.hpp> #include <adobe/serializable.hpp> /**************************************************************************************************/ namespace adobe { /**************************************************************************************************/ void test() { serializable_t value{array_t{}}; } /**************************************************************************************************/ } // namespace adobe

Without what line?

I don't think giving up on the reduction is appropriate. This configuration of this compiler applies all Adobe products and many others. IMO you should get to the point where you can submit a bug report. But that's just me.

Found it - a namespace lookup issue. Fixed property in array.hpp.

dabrahams

Some suggestions inline.

.vscode/settings.json

dabrahams · 2024-11-08T19:02:39Z

CMakeLists.txt

@@ -7,8 +7,7 @@ project(adobe_source_libraries CXX)

 set(CMAKE_CXX_EXTENSIONS OFF)
 if(NOT DEFINED CMAKE_CXX_STANDARD)
-    set(CMAKE_CXX_STANDARD 17)
-    set(CMAKE_XCODE_ATTRIBUTE_CLANG_CXX_LANGUAGE_STANDARD "c++17")
+    set(CMAKE_CXX_STANDARD 20)


I'm going to assume that dropping the Xcode setting is intentional and correct.

Yes. It isn't needed.

dabrahams · 2024-11-08T19:04:43Z

CMakePresets.json

+                "value": "x64",
+                "strategy": "external"
+            },
+            "toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",


So this toolchain requires vcpkg? Is that dependency documented in the README? If not you better do that.
Is how to use the presets documented?

The system is currently using vcpkg to pull in boost. It's undocumented, and I'd like to get rid of it in favor of fetch content, but not in this PR.

dabrahams · 2024-11-08T19:06:22Z

adobe/exception.hpp

+
+/**************************************************************************************************/
+
+/// invoke `std::terminate` with the given message.


? You can't invoke std::terminate with a message, so you need to fix this.

I updated the comment to be more clear.

dabrahams · 2024-11-08T19:07:57Z

adobe/implementation/toroid.hpp

@@ -227,7 +227,7 @@ class byte_toroid_t : boost::noncopyable {

    void purify(toroid_node_t* p) {
        toroid_header_t* c(column_of(p));
-        int x(p->color_m);
+        auto x(p->color_m);


Surely not needed for C++20 support? Why is it even an improvement?

Aside: we really don't use copy-initialization anymore? This syntax makes it look like a function declaration.

This silences a warning because x is char type and later uses will narrow back to a char type.

dabrahams · 2024-11-08T19:21:50Z

adobe/sha.hpp

-        stuff_bit_offset += num_bits;
-        n_stuffed_bits += num_bits;
+        ADOBE_ASSERT(num_bits < 8);
+        stuff_bit_offset += static_cast<std::uint16_t>(num_bits);


Maybe a comment explaining how you know it won't overflow is warranted.

I'm not an expert in this code - it took me a bit to figure out that num_bits < 8 here. I'm not certain, but it looks like stuff_bit_offset is always used in modulo value_bitsize_k and can warp (unsigned). n_stuffed_bits is 64 bits, so it would take a lot to overflow incrementing by 8. And I think this is going to be bounded by sizeof message_block_type * 8 - so you'll run out of VM space first (if not addressable space).

Well, if you can't prove it, there's always boost::numeric_cast to maintain safety.

That might be very bad if it is expected to wrap.

dabrahams · 2024-11-08T19:24:49Z

source/adam.cpp

@@ -1114,36 +1114,34 @@ void sheet_t::implementation_t::flow(cell_bits_t& priority_accessed) {
                index_t::iterator iter = output_index_m.find(term->name_set_m[n]);
                assert(iter != output_index_m.end());

-                cell_t& cell = *iter;
+                cell_t& e = *iter;


OMG this is a hecka long function. Nice getting rid of the type information in the name, but can we do better than e? What role is this variable playing on the inside of the (raw!) loop? That is, if you were to break the body of the loop into a function call taking cell_t&, what would you call the parameter?

I did a fair amount of refactoring and cleanup of that function and used better names. Still long - opened Issue#127 to revisit.

dabrahams · 2024-11-08T19:25:55Z

source/eve.cpp

-        for (guide_set_t::iterator first(boost::next(forward_guide_set.begin())),
-             last(forward_guide_set.end());
-             first != last; ++first) {
+        for (guide_set_t::iterator f(boost::next(forward_guide_set.begin())),


Is this an improvement?

Was a quick way to squelch a warning - but I resolved the revisit comment and moved the code to use std::generate()

dabrahams · 2024-11-08T19:27:32Z

source/eve_evaluate.cpp

-                for (array_t::const_iterator iter(spacing_array.begin());
-                     iter != spacing_array.end(); ++iter) {
-                    *dest_iter = iter->cast<int>();
+                for (array_t::const_iterator spacing_iter(spacing_array.begin());


I think a single character name would actually be clearer in this case. Also, though, no raw loops would alleviate the need to name these trivial things everywhere.

Replaced this one with transform()

dabrahams · 2024-11-08T19:32:22Z

test/property_model_eval/iomanip_flat.cpp

@@ -151,7 +151,7 @@ void flat_format::handle_atom(stream_type& os, bool is_push) {
        } else if (value.type_info() == typeid(dictionary_t)) {
            os << value.cast<dictionary_t>();
        } else if (value.type_info() == typeid(array_t)) {
-            os << value.cast<array_t>();
+            os << top.value().cast<::adobe::array_t>();


Probably you want to revert this change, but if not, comment to explain why this line needs to be so different from the rest.

Reducing loops in sheet flow Added append algorithm

dabrahams · 2024-11-12T18:54:38Z

adobe/algorithm/append.hpp

+/// Insert the range `r` at the end of the sequence container `x`.
+/// Returns an iterator pointing to the first element inserted, or `x.end()` if `r` is empty.
+template <class T, // T models SequenceContainer
+          class R> // R models Range
+inline auto append(T& x, const R& r) {


Suggested change

/// Insert the range `r` at the end of the sequence container `x`.

/// Returns an iterator pointing to the first element inserted, or `x.end()` if `r` is empty.

template <class T, // T models SequenceContainer

class R> // R models Range

inline auto append(T& x, const R& r) {

```suggestion

/// Inserts the elements of *range* `r` (a `Range`) at the end of *forward container* `x`,

/// returning the position corresponding to the incoming `end` of `x`.

template <class T, class R>

inline auto append(T& x, const R& r) {

dabrahams · 2024-11-12T18:57:08Z

source/adam.cpp

+    /// Returns the output cell with the given name.
+    /// \pre the output cell must exist.


Suggested change

/// Returns the output cell with the given name.

/// \pre the output cell must exist.

/// Returns the output cell with the given name.

///

/// \pre A cell with that name exists.

Blank line after summary.
A precondition is a predicate statement. Write in English what you'd put in the assert() parens.

dabrahams · 2024-11-12T19:01:02Z

source/adam.cpp

+        return *p;
+    }
+
+    /// Returns `true` if any output cells in the relation are resolved, false otherwise.


Some people think iff is too obscure, but this is an option

Suggested change

/// Returns `true` if any output cells in the relation are resolved, false otherwise.

/// Returns `true` iff any output cells in the relation are resolved.

Some people like:

Suggested change

/// Returns `true` if any output cells in the relation are resolved, false otherwise.

/// Returns whether any output cells in the relation are resolved.

I find that really awkward personally but I can see the argument.

If you are in the context of a type representing a relation, you can leave out “in the relation.”

My preference was iff (I had that initially, but I know people who would object, so I changed it). I'll go with "whether" -

Co-authored-by: Dave Abrahams <dabrahams@adobe.com>

sean-parent added 6 commits November 6, 2024 11:25

Updating cmake for fixes.

7ab7a4f

Update CMake configuration and improve code compatibility with C++20.…

900f620

… Compiling with MSVC at \W4.

git clang-format

5dd244f

Complete docs for macro_utilities.hpp

b670fc3

Fixing sscanf_s issue.

58c457b

Adding C++20 support for macos

d68084d

sean-parent requested review from dabrahams and camio November 7, 2024 06:01

sean-parent commented Nov 7, 2024

View reviewed changes

sean-parent added 4 commits November 6, 2024 22:37

Windows doesn't support __STDC_LIB_EXT1__ macro... sigh.

2e78a81

Fixing another warning in CI

622afeb

Removing variable only used in debug.

2e8b826

Hack to fix C++17 build.

0f69ed8

sean-parent commented Nov 8, 2024

View reviewed changes

Left in debugging code!

c93d480

dabrahams requested changes Nov 8, 2024

View reviewed changes

Addressing PR comments

32d3c7b

Reducing loops in sheet flow Added append algorithm

dabrahams reviewed Nov 12, 2024

View reviewed changes

sean-parent and others added 8 commits November 12, 2024 12:28

More PR feedback

8888b16

Fixing Issue#119 and more PR cleanup.

57e3f22

Fixing name lookup issue with operator<< and array_t.

01ff792

More PR comments addressed.

7a61a75

Two step removal.

1cb0b49

Don't sink vscode settings.

7d3c408

Using generate in eve.cpp and some boost removal.

a46be16

Update adobe/macro_utilities.hpp

ef7ca40

Co-authored-by: Dave Abrahams <dabrahams@adobe.com>

sean-parent merged commit ea5e048 into main Nov 13, 2024
9 checks passed

sean-parent deleted the msvc-cpp-20-support branch November 13, 2024 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSVC C++20 Support #126

MSVC C++20 Support #126

sean-parent commented Nov 7, 2024 •

edited

Loading

sean-parent Nov 7, 2024

camio Nov 8, 2024 •

edited

Loading

dabrahams Nov 8, 2024 •

edited

Loading

sean-parent Nov 12, 2024

sean-parent Nov 8, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams left a comment

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 8, 2024

sean-parent Nov 12, 2024

dabrahams Nov 8, 2024

sean-parent Nov 12, 2024

dabrahams Nov 12, 2024

sean-parent Nov 12, 2024

dabrahams Nov 8, 2024

sean-parent Nov 12, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 8, 2024

sean-parent Nov 13, 2024

dabrahams Nov 12, 2024

sean-parent Nov 13, 2024

dabrahams Nov 12, 2024

sean-parent Nov 13, 2024

dabrahams Nov 12, 2024

sean-parent Nov 13, 2024


		/**************************************************************************************************/

		/// invoke `std::terminate` with the given message.

		/// Returns the output cell with the given name.
		/// \pre the output cell must exist.

	/// Returns `true` if any output cells in the relation are resolved, false otherwise.
	/// Returns `true` iff any output cells in the relation are resolved.

	/// Returns `true` if any output cells in the relation are resolved, false otherwise.
	/// Returns whether any output cells in the relation are resolved.

MSVC C++20 Support #126

MSVC C++20 Support #126

Conversation

sean-parent commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

camio Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

dabrahams Nov 8, 2024 • edited Loading

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

dabrahams 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

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

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

sean-parent commented Nov 7, 2024 •

edited

Loading

camio Nov 8, 2024 •

edited

Loading

dabrahams Nov 8, 2024 •

edited

Loading