Quiet some warnings #2695

nicowilliams · 2023-07-10T23:29:49Z

No description provided.

itchyny · 2023-07-10T23:31:48Z

src/jv_dtoa.c

@@ -1723,6 +1724,7 @@ gethex(struct dtoa_context* C,  CONST char **sp, U *rvp, int rounding, int sign)
 		  case '-':
 			esign = 1;
 			/* no break */
+			JQ_FALLTHROUGH;


We no longer need no break comment?

Well, this isn't our code, and the no break comments never had any effect as far as compilers and linters go, and I think leaving them there as a bread-crumb on what to look for in history might be useful. Though, yeah, they serve no purpose now and you can still find them in the history, so I could drop those. Your call.

Hmm, maybe we shouldn't edit jv_dtoa.c for any purpose.

That is definitely an option, though it's been edited some -- first of all by @stedolan.

I couldn't find the original base code of dtoa.c when included into the repository, but I think the original diff was only jvp_ prefix and MALLOC/FREE definitions. The latest version is available at https://www.netlib.org/fp/ so I downloaded dtoa.c and g_fmt.c to compare against jv_dtoa.c, and there seems to be some bug fixes. I don't confirm jq will be affected by the fixes, but hopefully we could follow the upstream. Since it's just compiler warnings, it's better to avoid edits, report to the original author (maybe along with 6151f20), and then include the updates.

I remember finding the original and seeing that @stedolan made some modifications to the default representation of numbers. But it's been years since I looked.

https://www.google.com/search?q=dtoa.c+david+m+gay

itchyny · 2023-07-11T05:48:07Z

src/jv.h

+# define JQ_FALLTHROUGH DISPATCH_FALLTHROUGH
+#else
+# if defined(__GNUC__)
+#  if __GNUC__ >= 7


Can we just check __has_attribute(__fallthrough__)?

Sure. I'll update this PR tomorrow.

@nicowilliams ping :)

#if defined(DISPATCH_FALLTHROUGH) # define JQ_FALLTHROUGH DISPATCH_FALLTHROUGH #else # if defined __has_attribute # if __has_attribute(__fallthrough__) # define JQ_FALLTHROUGH __attribute__((fallthrough)) # endif # else # define JQ_FALLTHROUGH do {} while (0) /* fallthrough */ # endif #endif

look ok?

itchyny · 2023-07-11T05:49:36Z

src/jv.h

+#   define JQ_FALLTHROUGH do {} while (0) /* fallthrough */
+#  endif
+# else
+#  define JQ_FALLTHROUGH do {} while (0) /* fallthrough */


I found fallthrough macro in Linux code and the code using it looks nice, but should we use JQ_ prefix to avoid conflict?

@itchyny well, we need one that is portable to not-Linux, including BSDs, OS X, and Windows, as well as different compilers. I did struggle with: what header file to put this in, and what to call this macro.

It's very annoying that there isn't a universally available way to do this...

emanuele6 · 2023-07-16T03:15:06Z

This seems ok, but this PR doesn't silence all warnings:

src/util.c:3: warning: "_GNU_SOURCE" redefined                                       
    3 | #define _GNU_SOURCE               
      |                                                                              
<command-line>: note: this is the location of the previous definition

Since we are using AC_USE_SYSTEM_EXTENSIONS in configure.ac, on GNU/Linux, -D_GNU_SOURCE=1 will be passed to the compiler.

I think either wrapping that #define in #ifndef _GNU_SOURCE or just removing it should fix that.

nicowilliams · 2023-07-16T05:01:22Z

This seems ok, but this PR doesn't silence all warnings:

I have updated the caption! :)

itchyny · 2023-07-27T12:09:27Z

I thought __has_attribute(__fallthrough__) is the correct way of checking the availability of attributes, but after some observations in code search, I noticed some complicated situations of clang. Some implementations suggests me that on Clang < 10, __has_attribute(fallthrough) produces 1 even if it does not support C-style (but only supports C++-style) fallthrough attribute. Clang 10 is released on Mar 24, 2020, so we may need to support older versions. Since __has_attribute check is not reliable, I end up concluded that checking the version is the stable way.

#if (defined(__GNUC__) && __GNUC__ >= 7 && !defined(__clang__) || \
    defined(__clang__) && __clang_major__ >= 10)
# define JQ_FALLTHROUGH __attribute__((fallthrough))
#else
# define JQ_FALLTHROUGH
#endif

What do you think?

itchyny · 2023-07-27T11:14:15Z

src/jv.h

@@ -5,6 +5,18 @@
 #include <stdint.h>
 #include <stdio.h>

+#if defined(DISPATCH_FALLTHROUGH)
+# define JQ_FALLTHROUGH DISPATCH_FALLTHROUGH


Came from libdispatch? Are you sure?

I copied this from Heimdal :) The link is helpful. I'll update this.

nicowilliams · 2023-07-29T18:50:08Z

[...] I noticed some complicated situations of clang.

Bummer. ~~Your link shows use of JE_COMPILABLE to deal with this -- that strikes me as the best option, and I'll integrate that now.~~ I'll integrate a version of the code you proposed.

BTW, thank you for that research! I should have done that myself.

emanuele6 · 2023-07-30T02:08:35Z

src/jv_dtoa.c

@@ -2367,10 +2372,12 @@ jvp_strtod
 		case '-':
 			sign = 1;
 			/* no break */
+			JQ_FALLTHROUGH;
 		case '+':
 			if (*++s)
 				goto break2;
 			/* no break */


redundant comment

emanuele6 · 2023-07-30T02:08:57Z

src/jv_dtoa.c

@@ -3699,6 +3707,7 @@ jvp_dtoa
 		case 2:
 			leftright = 0;
 			/* no break */


redundant comment

emanuele6 · 2023-07-30T02:09:07Z

src/jv_dtoa.c

@@ -3707,6 +3716,7 @@ jvp_dtoa
 		case 3:
 			leftright = 0;
 			/* no break */


redundant comment

emanuele6 · 2023-07-30T02:09:18Z

src/jv_dtoa.c

@@ -2367,10 +2372,12 @@ jvp_strtod
 		case '-':
 			sign = 1;
 			/* no break */


redundant comment

emanuele6 · 2023-07-30T02:09:54Z

src/jv_dtoa.c

@@ -1723,6 +1724,7 @@ gethex(struct dtoa_context* C,  CONST char **sp, U *rvp, int rounding, int sign)
 		  case '-':
 			esign = 1;
 			/* no break */


redundant comment

nicowilliams · 2023-07-30T02:23:32Z

@emanuele6 I wanted to leave the original no break commentary in src/jv_dtoa.c.

emanuele6 · 2023-07-30T02:24:05Z

Ok, that's fine then

emanuele6 · 2023-07-30T02:26:02Z

Thanks!

nicowilliams marked this pull request as ready for review July 10, 2023 23:29

nicowilliams requested a review from itchyny July 10, 2023 23:30

itchyny reviewed Jul 10, 2023

View reviewed changes

itchyny reviewed Jul 11, 2023

View reviewed changes

nicowilliams changed the title ~~Quiet warnings~~ Quiet some warnings Jul 16, 2023

emanuele6 mentioned this pull request Jul 24, 2023

Remove all #define _GNU_SOURCE in source files #2761

Merged

nicowilliams force-pushed the quiet-warnings branch from 2124a02 to 679edf2 Compare July 24, 2023 16:44

emanuele6 requested a review from itchyny July 24, 2023 16:47

itchyny reviewed Jul 27, 2023

View reviewed changes

emanuele6 added this to the 1.7 release milestone Jul 28, 2023

nicowilliams added 2 commits July 29, 2023 17:27

Add JQ_FALLTHROUGH and use it to quiet warnings

595d97a

Use -Wno-cast-function-type to quiet many warnings

8ba22f6

nicowilliams force-pushed the quiet-warnings branch from 679edf2 to 8ba22f6 Compare July 29, 2023 22:27

emanuele6 reviewed Jul 30, 2023

View reviewed changes

emanuele6 merged commit f733a15 into jqlang:master Jul 30, 2023
28 checks passed

emanuele6 added the build label Jul 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quiet some warnings #2695

Quiet some warnings #2695

nicowilliams commented Jul 10, 2023

itchyny Jul 10, 2023

nicowilliams Jul 10, 2023

itchyny Jul 10, 2023

nicowilliams Jul 11, 2023

itchyny Jul 11, 2023 •

edited

Loading

nicowilliams Jul 16, 2023

nicowilliams Jul 16, 2023

itchyny Jul 11, 2023

nicowilliams Jul 16, 2023

emanuele6 Jul 24, 2023

nicowilliams Jul 24, 2023

itchyny Jul 11, 2023

nicowilliams Jul 16, 2023

emanuele6 commented Jul 16, 2023 •

edited

Loading

nicowilliams commented Jul 16, 2023

itchyny commented Jul 27, 2023 •

edited

Loading

itchyny Jul 27, 2023

nicowilliams Jul 29, 2023

nicowilliams commented Jul 29, 2023 •

edited

Loading

emanuele6 Jul 30, 2023

emanuele6 Jul 30, 2023

emanuele6 Jul 30, 2023

emanuele6 Jul 30, 2023

emanuele6 Jul 30, 2023

nicowilliams commented Jul 30, 2023

emanuele6 commented Jul 30, 2023

emanuele6 commented Jul 30, 2023

Quiet some warnings #2695

Quiet some warnings #2695

Conversation

nicowilliams commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itchyny Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuele6 commented Jul 16, 2023 • edited Loading

nicowilliams commented Jul 16, 2023

itchyny commented Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Jul 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicowilliams commented Jul 30, 2023

emanuele6 commented Jul 30, 2023

emanuele6 commented Jul 30, 2023

itchyny Jul 11, 2023 •

edited

Loading

emanuele6 commented Jul 16, 2023 •

edited

Loading

itchyny commented Jul 27, 2023 •

edited

Loading

nicowilliams commented Jul 29, 2023 •

edited

Loading