[small] tidy up warnings #2323

khushi-411 · 2023-09-13T05:58:32Z

Hi, the PR removes warnings during build time and tests. Please see:

/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c: In function ‘_lfortran_read_int64’:
/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c: In function ‘_lfortran_read_int64’:
/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c:1994:19: warning: format ‘%lld’ expects argument of type ‘long long int *’, but argument 2 has type ‘int64_t *’ {aka ‘long int *’} [-Wformat=]
 1994 |         scanf("%lld", p);
      |                ~~~^   ~
      |                   |   |
      |                   |   int64_t * {aka long int *}
      |                   long long int *
      |                %ld
/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c:1994:19: warning: format ‘%lld’ expects argument of type ‘long long int *’, but argument 2 has type ‘int64_t *’ {aka ‘long int *’} [-Wformat=]
 1994 |         scanf("%lld", p);
      |                ~~~^   ~
      |                   |   |
      |                   |   int64_t * {aka long int *}
      |                   long long int *
      |                %ld
/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c:2008:27: warning: format ‘%lld’ expects argument of type ‘long long int *’, but argument 3 has type ‘int64_t *’ {aka ‘long int *’} [-Wformat=]
 2008 |         fscanf(filep, "%lld", p);
      |                        ~~~^   ~
      |                           |   |
      |                           |   int64_t * {aka long int *}
      |                           long long int *
      |                        %ld
/home/khushi/Documents/lpython/src/libasr/runtime/lfortran_intrinsics.c:2008:27: warning: format ‘%lld’ expects argument of type ‘long long int *’, but argument 3 has type ‘int64_t *’ {aka ‘long int *’} [-Wformat=]
 2008 |         fscanf(filep, "%lld", p);
      |                        ~~~^   ~
      |                           |   |
      |                           |   int64_t * {aka long int *}
      |                           long long int *
      |                        %ld

and

warning: The symbol 'f' imported from modules_02b will shadow the existing symbol 'f'
 --> /home/khushi/Documents/lpython/integration_tests/modules_02.py:1:25
  |
1 | from modules_02b import f, f
  |                         ^ old symbol
  |
1 | from modules_02b import f, f
  |                            ^ new symbol

cc @certik @czgdp1807

Shaikh-Ubaid · 2023-09-13T06:47:39Z

Please enable -Werror for this file.

Also I think, now this shows warnings in macos. For example, from the build step in https://github.com/lcompilers/lpython/actions/runs/6168547819/job/16741232055?pr=2323:

/Users/runner/work/lpython/lpython/lpython-0.19.0-220-g8e845514a/src/libasr/runtime/lfortran_intrinsics.c:1994:22: warning: format specifies type 'long *' but the argument has type 'int64_t *' (aka 'long long *') [-Wformat]
        scanf("%ld", p);
               ~~~   ^
               %lld
/Users/runner/work/lpython/lpython/lpython-0.19.0-220-g8e845514a/src/libasr/runtime/lfortran_intrinsics.c:2008:30: warning: format specifies type 'long *' but the argument has type 'int64_t *' (aka 'long long *') [-Wformat]
        fscanf(filep, "%ld", p);
                       ~~~   ^
                       %lld
/Users/runner/work/lpython/lpython/lpython-0.19.0-220-g8e845514a/src/libasr/runtime/lfortran_intrinsics.c:1994:22: warning: format specifies type 'long *' but the argument has type 'int64_t *' (aka 'long long *') [-Wformat]
        scanf("%ld", p);
               ~~~   ^
               %lld
/Users/runner/work/lpython/lpython/lpython-0.19.0-220-g8e845514a/src/libasr/runtime/lfortran_intrinsics.c:2008:30: warning: format specifies type 'long *' but the argument has type 'int64_t *' (aka 'long long *') [-Wformat]
        fscanf(filep, "%ld", p);
                       ~~~   ^
                       %lld

khushi-411 · 2023-09-13T08:49:51Z

Ah, I see. You are right; there are warning messages in macos but not in ubuntu. I think it's related to the compilers.

Just to confirm this, I checked my previous PR; there were warning messages in ubuntu but not in macos. Please see: https://github.com/lcompilers/lpython/actions/runs/6164474892/job/16730254578

I also checked on the Godbolt and found that gcc and clang work pretty well, but there are warnings in msvc(windows). Link: https://godbolt.org/z/ba3vdTxeh.
.
I used PRId64 in my latest commit (de9bd28); let's see if all the build system are happy with it or not :-)

I mainly referred to this hack from stack-overflow.

Thanks!

Shaikh-Ubaid · 2023-09-13T08:53:28Z

Please also enable -Werror check for this file. I think that would ensure checking for warnings at the CI and thus help warning free builds in future.

certik · 2023-09-13T09:30:26Z

The runtime library must be build with -Werror and we need to ensure it fails on both macOS and Linux at the CI. That's a separate task.

Then we need to fix it. Yes, this %lld gives warnings on either macOS or Linux. We faced this before and I think I put some ifdefs, and use %lld on one, but %ld on the other, or something like that.

Thirumalai-Shaktivel · 2023-09-13T09:51:00Z

See:

lpython/src/libasr/runtime/lfortran_intrinsics.c

Lines 2598 to 2602 in df414fb

    
           #ifdef HAVE_LFORTRAN_MACHO 
        
                           DIM ", line %lld\n" S_RESET 
        
           #else 
        
                           DIM ", line %ld\n" S_RESET 
        
           #endif

khushi-411 · 2023-09-13T10:38:26Z

Interesting! Thanks for sharing. I have updated it using Macros.

EDIT: I see there are warnings on unused variables, I'm looking into it. Will update it soon. Thanks

This reverts commit 22c8474.

khushi-411 · 2023-09-15T06:58:29Z

Hi, @certik @Shaikh-Ubaid @Thirumalai-Shaktivel,

In LPython CI (3.10, ubuntu-latest), we are currently getting the following warning:

/home/runner/work/lpython/lpython/lpython-0.19.0-232-g05cd6c4a7/src/libasr/runtime/lfortran_intrinsics.c: In function ‘_lfortran_read_int32’:
/home/runner/work/lpython/lpython/lpython-0.19.0-232-g05cd6c4a7/src/libasr/runtime/lfortran_intrinsics.c:1972:9: warning: ignoring return value of ‘scanf’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 1972 |         scanf("%d", p);
      |         ^~~~~~~~~~~~~~

I stumbled upon an interesting discussion here. I found it is the expected behavior of the scanf function (because scanf is user-defined and the GCC compiler wants to warn the user about its input type).

I initially thought we might be getting this warning due to the unused variable p, I considered using [[maybe_unused]], I could have also used __attribute__((unused)) (for GCC and clang), but this case is not related to unused variable/function, etc.

What do you think we should do in these cases to remove the warning? I'd appreciate your response. Thank you!

Thirumalai-Shaktivel · 2023-09-15T07:05:46Z

I think we can try if (scanf("%d", p)) { } // Suppress unused result warning
But let's wait for other's opinions as well.

Shaikh-Ubaid · 2023-09-15T16:04:29Z

I think we can also disable the warning related to the unused result using the flag -Wno-unused-result only for the specific lfortran_intrinsics.c file.

certik · 2023-09-15T19:17:55Z

We can, but I think those are good warnings, and we should only disable them when we know the result value was meant to be ignored with the [[maybe_unused]] syntax. So I would keep the warning on.

khushi-411 · 2023-09-15T19:40:07Z

I think we can try if (scanf("%d", p)) { } // Suppress unused result warning
But let's wait for other's opinions as well.

I think we can also disable the warning related to the unused result using the flag -Wno-unused-result only for the specific lfortran_intrinsics.c file.

Thanks, @Thirumalai-Shaktivel and @Shaikh-Ubaid, for sharing two other ways to disable the warning message.

We can, but I think those are good warnings, and we should only disable them when we know the result value was meant to be ignored with the [[maybe_unused]] syntax. So I would keep the warning on.

Got it! Thanks, @certik, for clarifying it. It sounds totally reasonable to me.

This PR is ready by my side. Please let me know if there's anything to be improved in this PR.

certik

I think this looks good. We should probably use a different ifdef for apple (I am not sure if this define can be off sometimes for apple), but for now this is good.

certik · 2023-09-15T19:56:21Z

Thanks!

Shaikh-Ubaid · 2023-09-15T23:12:06Z

Thank you so much @khushi-411. Great work! I appreciate it.

Shaikh-Ubaid · 2023-09-15T23:14:41Z

I used PRId64 in my latest commit (de9bd28); let's see if all the build system are happy with it or not :-)

The above seems to be a correct approach. Please see https://chat.openai.com/share/51fe5b13-86a1-4ebe-809d-a71d7d284cab. It seems for scanf() it is SCNd64 and for printf() it is PRId64.

Shaikh-Ubaid · 2023-09-15T23:32:45Z

For the unused result warnings, it seems we also have the possibility/approach of casting the result to void (when we do not need/use the result). For example,

$ cat examples/main.c 
#include <stdio.h>

__attribute__((warn_unused_result))
int custom_function(int value) {
    return value * 2;
}

int main() {
    custom_function(5); // This will trigger a warning for unused result
    return 0;
}

$ clang -Wunused-result -o unused_result examples/main.c
examples/main.c:9:5: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
    custom_function(5); // This will trigger a warning for unused result
    ^~~~~~~~~~~~~~~ ~
1 warning generated.

Casting the result to void like

int main() {
    (void) custom_function(5);
    return 0;
}

$ clang -Wunused-result -o unused_result examples/main.c
$

PS: It seems to work on mac. We might need to check on Linux.

khushi-411 · 2023-09-16T05:53:30Z

It seems for scanf() it is SCNd64 and for printf() it is PRId64.

Ah, right. My bad, I missed it. Thanks for pointing it out, @Shaikh-Ubaid! :-)
I think it might have worked earlier but would have shown some miscellaneous effect while dtype conversion. Happy we haven't used that.

PS: It seems to work on mac. We might need to check on Linux.

Nice approach!

I tested it on Ubuntu, and it appears clang is happy, but gcc still throws the same warning msg. Please see:

(lp) khushi@khushi:~/Documents/lpython$ clang -Wunused-result -o result main.c
(lp) khushi@khushi:~/Documents/lpython$
(lp) khushi@khushi:~/Documents/lpython$ gcc -Wunused-result -o result main.c
main.c: In function ‘main’:
main.c:9:12: warning: ignoring return value of ‘custom_function’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
    9 |     (void) custom_function(5); // This will trigger a warning for unused result
      |            ^~~~~~~~~~~~~~~~~~

[small] tidy up warnings

34a6461

Shaikh-Ubaid marked this pull request as draft September 13, 2023 06:48

use PRTd64

de9bd28

use macros

fb6b2ec

khushi-411 added 6 commits September 13, 2023 16:09

Merge branch 'main' into remove_warns

6ef41e9

update refs

c069e61

oops: typo

fa24e0f

remove \n

d0e9ad5

use [[maybe_unused]]

22c8474

Revert "use [[maybe_unused]]"

4d42172

This reverts commit 22c8474.

certik marked this pull request as ready for review September 15, 2023 19:55

certik approved these changes Sep 15, 2023

View reviewed changes

certik merged commit 825575a into lcompilers:main Sep 15, 2023
12 checks passed

khushi-411 deleted the remove_warns branch September 15, 2023 19:58

khushi-411 mentioned this pull request Oct 15, 2023

Linux: warnings when building on linux lfortran/lfortran#2568

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[small] tidy up warnings #2323

[small] tidy up warnings #2323

khushi-411 commented Sep 13, 2023 •

edited

Loading

Shaikh-Ubaid commented Sep 13, 2023

khushi-411 commented Sep 13, 2023 •

edited

Loading

Shaikh-Ubaid commented Sep 13, 2023

certik commented Sep 13, 2023

Thirumalai-Shaktivel commented Sep 13, 2023

khushi-411 commented Sep 13, 2023 •

edited

Loading

khushi-411 commented Sep 15, 2023

Thirumalai-Shaktivel commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023

certik commented Sep 15, 2023 •

edited

Loading

khushi-411 commented Sep 15, 2023

certik left a comment

certik commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023 •

edited

Loading

Shaikh-Ubaid commented Sep 15, 2023 •

edited

Loading

khushi-411 commented Sep 16, 2023 •

edited

Loading

[small] tidy up warnings #2323

[small] tidy up warnings #2323

Conversation

khushi-411 commented Sep 13, 2023 • edited Loading

Shaikh-Ubaid commented Sep 13, 2023

khushi-411 commented Sep 13, 2023 • edited Loading

Shaikh-Ubaid commented Sep 13, 2023

certik commented Sep 13, 2023

Thirumalai-Shaktivel commented Sep 13, 2023

khushi-411 commented Sep 13, 2023 • edited Loading

khushi-411 commented Sep 15, 2023

Thirumalai-Shaktivel commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023

certik commented Sep 15, 2023 • edited Loading

khushi-411 commented Sep 15, 2023

certik left a comment

Choose a reason for hiding this comment

certik commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023

Shaikh-Ubaid commented Sep 15, 2023 • edited Loading

Shaikh-Ubaid commented Sep 15, 2023 • edited Loading

khushi-411 commented Sep 16, 2023 • edited Loading

khushi-411 commented Sep 13, 2023 •

edited

Loading

khushi-411 commented Sep 13, 2023 •

edited

Loading

khushi-411 commented Sep 13, 2023 •

edited

Loading

certik commented Sep 15, 2023 •

edited

Loading

Shaikh-Ubaid commented Sep 15, 2023 •

edited

Loading

Shaikh-Ubaid commented Sep 15, 2023 •

edited

Loading

khushi-411 commented Sep 16, 2023 •

edited

Loading