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

[RFE] allow assign to members of rvalue aggregate (-fpermissive) #40016

Open
llvmbot opened this issue Feb 8, 2019 · 16 comments
Open

[RFE] allow assign to members of rvalue aggregate (-fpermissive) #40016

llvmbot opened this issue Feb 8, 2019 · 16 comments
Labels
bugzilla Issues migrated from bugzilla c++

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2019

Bugzilla Link 40670
Version unspecified
OS Linux
Reporter LLVM Bugzilla Contributor
CC @DougGregor,@zygoloid

Extended Description

#include <iostream>
#include <cstdlib>
class s {
public:
    int a;
    ~s() { std::cout << "a=" << a << std::endl; }
};
static s foo()
{
    return s{0};
}
int main()
{
    foo().a = 5;
    int *p = std::malloc(sizeof(int));
    return *p;
}

This code can be compiled with g++ if -fpermissive is used.
clang++ doesn't compile it with any options.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Feb 8, 2019

https://lkml.org/lkml/2018/9/13/1272
LKML people also seem to want -fpermissive, and
overall google shows quite a lot of demand.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Feb 11, 2019

This example demonstrates permissive conversion from
void*, and also an extension to write fields of an aggregate
returned as rvalue. This is a useful extension, because
in the destructor you can have the use of the altered fields,
as the example demonstrates. This is a main motivation why
I think -fpermissive should be supported (permissive conversions
are not so important). g++ supports both, and probably more
extensions under -fpermissive (can't find the exact docs).

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Feb 12, 2019

A general "support -fpermissive" bug is too vague to be actionable -- -fpermissive covers a lot of GNU extensions, some of which we already implement, some of which might be useful but are not currently supported in Clang, and others of which we might want to specifically take a stand against implementing.

Can we repurpose this bug to specifically be about assigning to a subobject of a temporary?

(Incidentally, we should likely also add direct support for a -fpermissive flag, and make it downgrade all of our error-by-default extensions into warnings.)

@llvmbot
Copy link
Collaborator Author

llvmbot commented Feb 12, 2019

Can we repurpose this bug to specifically be about assigning to a subobject
of a temporary?

Not sure what is the best way of repurpose.
I changed the description.
But will it be activated with -fpermissive, or otherwise?

(Incidentally, we should likely also add direct support for a -fpermissive
flag, and make it downgrade all of our error-by-default extensions into
warnings.)

Would be good.
AFAIK this is all it does.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Feb 12, 2019

By the way,
https://en.cppreference.com/w/cpp/language/implicit_conversion#Temporary_materialization

The standard seems to say that a temporary materialization
occurs "when performing a member access on a class prvalue;",
which seems to be the case here.
So I even wonder why it is not assignable by default?
Is materialization not sufficient to became assignable?

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Feb 12, 2019

Temporary materialization conversion converts the s prvalue to an s xvalue. Class member access then gives an int xvalue. Assignment needs an lvalue; an xvalue won't do.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@stsp
Copy link

stsp commented May 13, 2023

Hi, what would be a resolution of this?
Since clang cares that much with gcc
compatibility recently, would it be possible
to implement that gcc extension in clang?
If -fpermissive is a bad name, then some
-fassignable-xvalue can be used, or alike.

@shafik
Copy link
Collaborator

shafik commented May 15, 2023

This request does not seem particularly strongly motivated at least as explained currently. This case can covered using designated initializers e.g. s{.a=10}, full example: https://godbolt.org/z/Ea78vG3Ex

#include <iostream>
#include <cstdlib>
class s {
public:
    int a;
    ~s() { std::cout << "a=" << a << std::endl; }
};
static s foo()
{
    return s{0};
}
int main()
{
    (void)s{.a=10};
    int *p = (int*)std::malloc(sizeof(int));
    return *p;
}

If we do a have a more strongly motivated case, I believe a RFC on Discourse would be more appropriate today.

CC @AaronBallman

@stsp
Copy link

stsp commented May 16, 2023

If it would be a new code then maybe
a different syntax would be possible.
But currently I need to deal with a huge
code that does foo().a = 5; as shown
in my example.
Its just that currently foo is defined
this way:

wrp_type& foo() { return *new(get_buf()) wrp_type; }

which means the destructor is never called
implicitly. I wanted to change the definition
of foo this way:

wrp_type foo() { return wrp_type(get_buf()); }

in which case after
foo().a = 5; the destructor would be called.
Unfortunately that works only on gcc.

Basically speaking, there are many syntaxes
that can be replaced with something else in
theory. But in practice you are not going to
rewrite the entire code base or change the
API you already provided your users with,
so you want some particular syntax even if
the problem can be solved differently.

@AaronBallman
Copy link
Collaborator

We've traditionally been strongly opposed to -fpermissive in Clang, and I don't think that position has changed. This is effectively asking for part of -fpermissive's functionality, which we've considered in the past, but generally only when the code shows up in some popular third-party library or other scenario where changing the code is effectively impossible. It doesn't sound to me like that's the case here. So I don't think this is a motivated change, unless I'm missing some information.

@stsp
Copy link

stsp commented May 16, 2023

I don't think my code represent a popular
third-party library (only about ~50-100 clones
in 2 weeks, per github stats), and I don't think
"changing the code is effectively impossible"
is my case as well because currently my
code is clang-specific anyway. Its just that
the recent clang-16 changes scared me a
lot so I started to think about the gcc porting.
gcc porting will require that functionality and
I don't want to became clang-incompatible
in a process.

Anyway, I don't propose to judge my code.
I propose to evaluate the extension itself.
To me, this looks like a tiny extension that
only requires to remove some check that
prevents the assignment to temporary from
happening. If that extension is really as
small as I think of it, and if both compilers
support it, then what prevents it from becoming
the part of the standard? :)
But if you think this is a difficult to implement
extension or is completely useless, then of
course its another story.

This is effectively asking for part of -fpermissive's functionality,
which we've considered in the past

Please provide me with an URL of such a
discussion, if possible.

@AaronBallman
Copy link
Collaborator

But if you think this is a difficult to implement
extension or is completely useless, then of
course its another story.

"Completely useless" is a bit heavier of a term than I'd use, but it's still pretty unmotivated (at least, to me). One of the criteria for adding extensions to Clang (https://clang.llvm.org/get_involved.html#criteria) is that there be evidence of a significant user community. I think that's the primary thing that I'm missing which would motivate me.

Please provide me with an URL of such a
discussion, if possible.

Oddly, this came up literally this morning: #54120 -- but there are other mentions of this on the mailing lists and bug database. Also: https://reviews.llvm.org/D58154

@stsp
Copy link

stsp commented May 16, 2023

Thanks.
Those are about an -fpermissive itself.
If most of its extensions are ugly and
require AST changes, then of course
they shouldn't be added.

This particular extension wasn't discussed
though. To me it looks very small and
natural, I am not even sure why it was
restricted by the standard to not work
that way. So it would be good to at least
find out your opinion first on whether
its "ugly" or not, as you marked another
extensions that way. If you think its among
them, then why to even bother. I only
want a gcc port of my code, so I want to
use the "good" extensions. :)

@AaronBallman
Copy link
Collaborator

This particular extension wasn't discussed though.

Correct, I've not seen a discussion of this particular aspect aside from on this thread.

To me it looks very small and natural, I am not even sure why it was restricted by the standard to not work that way.

See: #40016 (comment); the value category is important in terms of correctness, move semantics, and optimization opportunities.

So it would be good to at least find out your opinion first on whether its "ugly" or not, as you marked another extensions that way. If you think its among them, then why to even bother. I only want a gcc port of my code, so I want to use the "good" extensions. :)

I don't think it's a good extension to add. There are very, very few circumstances under which allowing assignment into an xvalue is reasonable, so adding this extension increases the chances of running into more bugs in order to work around what amounts to a design issue. You've found one of the cases where you could make an argument that the assignment is reasonable, but I think this is a quite rare situation, so it's not that compelling. Essentially, I don't think this meets the bar for an extension we'd be interested in adding to Clang, at least without further evidence that there is a greater need.

(To be clear, this is not "no, we'll never do it" to this particular request -- this is "no, we need more evidence that there's sufficient community need for a new language dialect".)

@stsp
Copy link

stsp commented May 16, 2023

so adding this extension increases the chances of running into more bugs
in order to work around what amounts to a design issue.

Its not a design issue. Its that gcc is
exceptionally buggy and most of all
porting ways I could think of, are blocked
by its bugs. See this one as a most recent
example:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109824
Some porting work is blocked by stalled
c++ proposals, for example I was putting
lots of hopes into this:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0352r1.pdf
but it never materialized.
So eventually I went for clang-only code,
and it was a huge relief.

evidence that there's sufficient community need for a new language dialect".)

This is almost a chicken-and-egg problem:
unless you implement that, no one will use. :)
But OK, the fact that its a bad extension on
your opinion, is spoken.

@stsp
Copy link

stsp commented May 16, 2023

There are very, very few circumstances under which allowing assignment into
an xvalue is reasonable

I think the correction is needed here:
to xvalue member of an aggregate
temporary object. I bet this has much
more chances to have a reasonable
semantic, than just a plain assignment
to some xvalue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++
Projects
None yet
Development

No branches or pull requests

4 participants