Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(parse): enable compiling with Libc++ in C++23+ #818

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

JohelEGP
Copy link
Contributor

This works around possibly llvm/llvm-project#58206,
as pointed out at #462 (comment).
So now we can compile cppfront with Libc++ in C++23+.

@hsutter
Copy link
Owner

hsutter commented Nov 13, 2023

Thanks! I'm not sure these are actual order dependency bugs in parse.h, but anyway it does seems like there's always another order dependency (or three) to be found in parse.h and I'm looking forward to converting that file in particular to Cpp2... 😏

@hsutter hsutter merged commit c840548 into hsutter:main Nov 13, 2023
@JohelEGP JohelEGP deleted the libcxx-c++23 branch November 13, 2023 04:00
@JohelEGP
Copy link
Contributor Author

and I'm looking forward to converting that file in particular to Cpp2...

I'm also looking forward to it!
Specially when you start using @union instead of std::variant.
That might have some interesting effects in compile-times and more.

zaucy pushed a commit to zaucy/cppfront that referenced this pull request Dec 5, 2023
@JohelEGP
Copy link
Contributor Author

I'm not sure these are actual order dependency bugs in parse.h

Seems like they are in C++23.
https://www.reddit.com/r/cpp/comments/18ls1ml/comment/ke080ew/?utm_source=share&utm_medium=web2x&context=3 links to issues for https://github.com/microsoft/STL and https://github.com/llvm/llvm-project.
They further link to related issues.
In the LLVM one, Richard Smith replies

The unique_ptr destructor becomes constexpr in C++23, so gets instantiated more eagerly -- at the point of first reference rather than at end of TU. unique_ptr destruction doesn't support incomplete types.

@hsutter
Copy link
Owner

hsutter commented Dec 29, 2023

I tried with GCC 13 in C++23 mode but wasn't able to find such a problem.

It did give one error though, which no other compiler reported and I can't see if there's really a problem... the error is:

In file included from sema.h:21,
                 from to_cpp1.h:21,
                 from cppfront.cpp:18:
reflect.h2: In instantiation of ‘bool cpp2::meta::apply_metafunctions(cpp2::declaration_node&, type_declaration&, const auto:162&) [with auto:162 = cpp2::parser::apply_type_metafunctions(cpp2::declaration_node&)::<lambda(const std::string&)>]’:
sema.h:35:31:   required from here
reflect.h2:1372:9: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
 1372 |         for meta*.template_arguments()
      |         ^~~
reflect.h2:1373:61: note: the temporary was destroyed at the end of the full expression ‘<lambda closure object>cpp2::meta::apply_metafunctions<cpp2::parser::apply_type_metafunctions(cpp2::declaration_node&)::<lambda(const std::string&)> >(cpp2::declaration_node&, type_declaration&, const cpp2::parser::apply_type_metafunctions(cpp2::declaration_node&)::<lambda(const std::string&)>&)::<lambda(Obj&&, Params&& ...)>().cpp2::meta::apply_metafunctions<cpp2::parser::apply_type_metafunctions(cpp2::declaration_node&)::<lambda(const std::string&)> >(cpp2::declaration_node&, type_declaration&, const cpp2::parser::apply_type_metafunctions(cpp2::declaration_node&)::<lambda(const std::string&)>&)::<lambda(Obj&&, Params&& ...)>((* &(& cpp2::assert_not_null<const std::unique_ptr<id_expression_node>&>((* & meta)))->std::unique_ptr<cpp2::id_expression_node>::operator*()))’
 1373 |         do  (arg)

The reflect.h2 code is:

        for meta*.template_arguments()
        do  (arg)
            args.push_back( arg.to_string() );

which is lowered to this:

        for ( 
             auto const& arg : CPP2_UFCS(template_arguments)((*cpp2::assert_not_null(meta))) ) 
            CPP2_UFCS(push_back)(args, CPP2_UFCS(to_string)(arg));

You last took a pass at the UFCS macros... do you see any actual possible dangling that could be happening?

@JohelEGP
Copy link
Contributor Author

I reduced the lambda to just

[&]<typename Obj> [[nodiscard]] (Obj && obj)
                    ->decltype(auto)  {
              return std::forward<decltype(obj)>(obj).template_arguments();
          }((*cpp2::assert_not_null(meta)))

It looks fine.
This must be one of these:

@JohelEGP
Copy link
Contributor Author

The effect of -Werror on the warning can be disabled with -Wno-error=dangling-reference.

hsutter added a commit that referenced this pull request Jan 4, 2024
Make grammar productions all <90 characters (they mostly were anyway, and some long names just weren't needed)

I changed some that ended in _-list_ to just end in _s_ (for plural), but left some like _expression-list_ because they were already short and read better with a dash (I think _expression-list_ reads better than _expressions_, YMMV)

Also added `-Wno-error=dangling-reference` to avoid a false positive in GCC 13; see #818 (comment)
@hsutter
Copy link
Owner

hsutter commented Jan 4, 2024

Done in 9585ecc

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jan 4, 2024

This most closely matches your error: https://stackoverflow.com/questions/77101074/possible-false-positive-from-wdangling-reference-using-g.
For it they opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111410#c0.
Looks like only GCC 13 accepts -Wno-error=dangling-reference: https://cpp1.godbolt.org/z/qYEcea77E.
All Unix CI jobs are failing due to that.

@hsutter
Copy link
Owner

hsutter commented Jan 4, 2024

Thanks, I just noticed that too and pushed 9aefcee

Update: Yes, that seems to have fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants