llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.91k stars 11.52k forks source link

elvis operator (?:) type conversion #88396

Open bsbernd opened 5 months ago

bsbernd commented 5 months ago

I know that the ?: is not standard, but llvm/clang supports it, afaik. But somehow it seems to convert the type

bschubert2@imesrv6 test>cat elvis.cpp
enum my_status_t {
    MY_SUCCESS = 0,
    MY_FAIL1 = 1,
    MY_FAIL2 = 2,

};

my_status_t func1()
{
    return MY_SUCCESS;
}

my_status_t func2()
{
    return MY_FAIL1;
}

int main(int argc, char **argv) 
{ 
    (void)argc;
    (void)argv;

    my_status_t status = func2() ?: func1();

    return (status == MY_SUCCESS) ? 0 : -1;
}
bschubert2@imesrv6 test>g++ -g -W -Wall -Werror -o elvis elvis.cpp
bschubert2@imesrv6 test>echo $?
0
bschubert2@imesrv6 test>clang++ -g -W -Wall -Werror -o elvis elvis.cpp
elvis.cpp:24:17: error: cannot initialize a variable of type 'my_status_t' with an rvalue of type 'int'
    my_status_t status = func2() ?: func1();
                ^        ~~~~~~~~~~~~~~~~~~
1 error generated.

Why is it converting from my_status_t to int?

llvmbot commented 5 months ago

@llvm/issue-subscribers-clang-frontend

Author: Bernd Schubert (bsbernd)

I know that the `?:` is not standard, but llvm/clang supports it, afaik. But somehow it seems to convert the type ``` bschubert2@imesrv6 test>cat elvis.cpp enum my_status_t { MY_SUCCESS = 0, MY_FAIL1 = 1, MY_FAIL2 = 2, }; my_status_t func1() { return MY_SUCCESS; } my_status_t func2() { return MY_FAIL1; } int main(int argc, char **argv) { (void)argc; (void)argv; my_status_t status = func2() ?: func1(); return (status == MY_SUCCESS) ? 0 : -1; } ``` ``` bschubert2@imesrv6 test>g++ -g -W -Wall -Werror -o elvis elvis.cpp bschubert2@imesrv6 test>echo $? 0 ``` ``` bschubert2@imesrv6 test>clang++ -g -W -Wall -Werror -o elvis elvis.cpp elvis.cpp:24:17: error: cannot initialize a variable of type 'my_status_t' with an rvalue of type 'int' my_status_t status = func2() ?: func1(); ^ ~~~~~~~~~~~~~~~~~~ 1 error generated. ``` Why is it converting from my_status_t to int?
shafik commented 5 months ago

Confirmed: https://godbolt.org/z/q99jYqr3E

This look wrong to me.

erichkeane commented 5 months ago

Yep, this looks like a bug to me. The problem is coming from here somewhere:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExpr.cpp#L9766

Basically, while we are doing the work to make the condition into the LHSExpr we are doing UsualUnaryConversions for some reason. I don't know why, but it likely needs some looking into.

The condition before it is pretty odd as well, I'm not sure what it is getting at, nor does it look like it matches the comment? Someone should probably spend some time looking into that bit of code's history and context and see if we can figure out what went wrong there.

bsbernd commented 5 months ago
commit c07a0c7e483cf6b157295dcc18bfb782e3424591 (HEAD)
Author: John McCall <rjmccall@apple.com>
Date:   Thu Feb 17 10:25:35 2011 +0000

    Change the representation of GNU ?: expressions to use a different expression
    class and to bind the shared value using OpaqueValueExpr.  This fixes an
    unnoticed problem with deserialization of these expressions where the
    deserialized form would lose the vital pointer-equality trait;  or rather,
    it fixes it because this patch also does the right thing for deserializing
    OVEs.

    Change OVEs to not be a "temporary object" in the sense that copy elision is
    permitted.

    This new representation is not totally unawkward to work with, but I think
    that's really part and parcel with the semantics we're modelling here.  In
    particular, it's much easier to fix things like the copy elision bug and to
    make the CFG look right.

    I've tried to update the analyzer to deal with this in at least some
    obvious cases, and I think we get a much better CFG out, but the printing
    of OpaqueValueExprs probably needs some work.

    llvm-svn: 125744