[PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Mark Wielaard]

Translating the AST LifetimeType to the HIR LifetimeType causes a warning:
warning: ‘ltt’ may be used uninitialized

Add a default clause to the switch statement calling gcc_unreachable.
This will suppress the warning and make sure that if the AST lifetime type
is invalid or a new unknown type we immediately know when lowering the AST.
---
 gcc/rust/hir/rust-ast-lower-type.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/rust/hir/rust-ast-lower-type.h b/gcc/rust/hir/rust-ast-lower-type.h
index 5e19850e8dd..568a806f98d 100644
--- a/gcc/rust/hir/rust-ast-lower-type.h
+++ b/gcc/rust/hir/rust-ast-lower-type.h
@@ -281,6 +281,8 @@ public:
       case AST::Lifetime::LifetimeType::WILDCARD:
 	ltt = HIR::Lifetime::LifetimeType::WILDCARD;
 	break;
+      default:
+	gcc_unreachable ();
       }
 
     HIR::Lifetime lt (mapping, ltt, param.get_lifetime ().get_lifetime_name (),
-- 
2.32.0

Re: [PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Marc]

Hi,

> Translating the AST LifetimeType to the HIR LifetimeType causes a warning:
> warning: ‘ltt’ may be used uninitialized

Was wondering why this is needed as the switch case covers all enum
variants, how can ltt be uninitialized ? I have the same fix locally but
was thinking something else was causing the error...

Marc

Re: [PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Mark Wielaard]

On Tue, Jun 29, 2021 at 12:06:56AM +0200, Marc wrote:
> Hi,
> 
> > Translating the AST LifetimeType to the HIR LifetimeType causes a warning:
> > warning: ‘ltt’ may be used uninitialized
> 
> Was wondering why this is needed as the switch case covers all enum
> variants, how can ltt be uninitialized ? I have the same fix locally but
> was thinking something else was causing the error...

LifetimeType is a plain enum, which aren't really their own types,
they are really just ints with fancy names. We could make them enum
class, which is a strong type. Then the compiler would know the switch
really covers all enum (class) variants. But then we have to provide
the right scope/type everywhere we use them in the code (which might
be a good idea, but is more typing).

Cheers,

Mark

Re: [PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Mark Wielaard]

On Tue, Jun 29, 2021 at 12:47:07AM +0200, Mark Wielaard wrote:
> On Tue, Jun 29, 2021 at 12:06:56AM +0200, Marc wrote:
> > Hi,
> > 
> > > Translating the AST LifetimeType to the HIR LifetimeType causes a warning:
> > > warning: ‘ltt’ may be used uninitialized
> > 
> > Was wondering why this is needed as the switch case covers all enum
> > variants, how can ltt be uninitialized ? I have the same fix locally but
> > was thinking something else was causing the error...
> 
> LifetimeType is a plain enum, which aren't really their own types,
> they are really just ints with fancy names. We could make them enum
> class, which is a strong type. Then the compiler would know the switch
> really covers all enum (class) variants. But then we have to provide
> the right scope/type everywhere we use them in the code (which might
> be a good idea, but is more typing).

I just tried to make LifetimeType an enum class and that doesn't help.
So I was wrong. I don't know why the compiler doesn't see this? It
should know since if not all switch cases were covered, -Wswitch
(enabled by -Wall) gives us a warning... So, I don't fully understand
why gcc needs the default gcc_unreachable case. It is what is used in
the rest of the code though.

Cheers,

Mark

Re: [PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Marc]

Mark Wielaard <mark@klomp.org> writes:

> On Tue, Jun 29, 2021 at 12:47:07AM +0200, Mark Wielaard wrote:
>> On Tue, Jun 29, 2021 at 12:06:56AM +0200, Marc wrote:
>> > Hi,
>> > 
>> > > Translating the AST LifetimeType to the HIR LifetimeType causes a warning:
>> > > warning: ‘ltt’ may be used uninitialized
>> > 
>> > Was wondering why this is needed as the switch case covers all enum
>> > variants, how can ltt be uninitialized ? I have the same fix locally but
>> > was thinking something else was causing the error...
>> 
>> LifetimeType is a plain enum, which aren't really their own types,
>> they are really just ints with fancy names. We could make them enum
>> class, which is a strong type. Then the compiler would know the switch
>> really covers all enum (class) variants. But then we have to provide
>> the right scope/type everywhere we use them in the code (which might
>> be a good idea, but is more typing).
>
> I just tried to make LifetimeType an enum class and that doesn't help.
> So I was wrong. I don't know why the compiler doesn't see this? It
> should know since if not all switch cases were covered, -Wswitch
> (enabled by -Wall) gives us a warning... So, I don't fully understand
> why gcc needs the default gcc_unreachable case. It is what is used in
> the rest of the code though.

I thought maybe that's a C++ vs C diff, or something caused by the
Lifetime being returned by a function call, but I can't reproduce it, so
that must be something else:

https://godbolt.org/z/sjbcWEqdj

Anyway, Philipp wants to have these enum shared between AST and HIR, so
this kind of 'if(AST::Foo) t=HIR::Foo' can be removed.

I'm still curious about why the warning is raised in this case...

Marc

Re: [PATCH] Suppress warning in rust-ast-lower-type.h ASTLowerGenericParam.visit.

[Mark Wielaard]

Hi Marc,

On Tue, Jun 29, 2021 at 08:28:51AM +0200, Marc wrote:
> Mark Wielaard <mark@klomp.org> writes:
> > I just tried to make LifetimeType an enum class and that doesn't help.
> > So I was wrong. I don't know why the compiler doesn't see this? It
> > should know since if not all switch cases were covered, -Wswitch
> > (enabled by -Wall) gives us a warning... So, I don't fully understand
> > why gcc needs the default gcc_unreachable case. It is what is used in
> > the rest of the code though.
> 
> I thought maybe that's a C++ vs C diff, or something caused by the
> Lifetime being returned by a function call, but I can't reproduce it, so
> that must be something else:
> 
> https://godbolt.org/z/sjbcWEqdj

Try using the result of the function and using -O2

  enum LifetimeType
  {
    NAMED,   // corresponds to LIFETIME_OR_LABEL
    STATIC,  // corresponds to 'static
    WILDCARD // corresponds to '_
  };

 int g(int i);

 LifetimeType toto();
  int t ()  {
      int t;
      switch(toto()){
          case NAMED:
          t=4;
          break;
          case STATIC:
          t=5;
          break;
          case WILDCARD:
          t=8;
          break;
      }
      return g(t);
  }

gcc -O2 -Wall

<source>: In function 'int t()':
<source>:24:15: warning: 't' may be used uninitialized in this function [-Wmaybe-uninitialized]
   24 |       return g(t);
      |              ~^~~

> Anyway, Philipp wants to have these enum shared between AST and HIR, so
> this kind of 'if(AST::Foo) t=HIR::Foo' can be removed.

That might be a good idea if the LifetimeType has the same values and
semantics between AST and HIR.

But till that happend I think it is a good idea to suppress warnings like this.

Cheers,

Mark