Skip to content

configure: tidy up comments#21246

Closed
vszakats wants to merge 16 commits into
curl:masterfrom
vszakats:am-comments
Closed

configure: tidy up comments#21246
vszakats wants to merge 16 commits into
curl:masterfrom
vszakats:am-comments

Conversation

@vszakats
Copy link
Copy Markdown
Member

@vszakats vszakats commented Apr 6, 2026

  • convert # comments to dnl, except copyright headers, and inline
    comments in curl-complilers.m4.
  • drop empty comments.
  • drop line-ending dnl markers. (except zz40-xc-ovr.m4 where it's
    used to produce a comment in configure.)
  • replace dnl line with C comment in AC_CHECK_HEADERS().

Verified to produce the same configure script except empty lines,
# comments, and C comments, with autoreconf 2.72.

Cherry-picked from #21000


  • drop dnl# changes.

@vszakats vszakats marked this pull request as draft April 6, 2026 22:06
@github-actions github-actions Bot added the build label Apr 6, 2026
@dfandrich
Copy link
Copy Markdown
Contributor

dfandrich commented Apr 7, 2026 via email

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Apr 7, 2026

The idea is to use the same comment-style in all files. The patch set first switches
everything to dnl, then switches all of them to #.

The configure size advantage with dnl is 6% (88KB). While it also makes
it more readable due to the retained comments. # is less weird and more
familiar (IMO) than dnl, and supports inline comments.

I prefer # somewhat more. We can go either way.

@bagder
Copy link
Copy Markdown
Member

bagder commented Apr 8, 2026

personally, I don't think unifying the comments is worth the work. I don't think they cause trouble to anyone

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Apr 8, 2026

The work is already done, but since I sense we'd prefer staying with dnl,
I'll rewind to the point where # comments are swapped to dnl (except
for the rare inline ones), drop a bunch of empty comments and
accidental-looking line-ending dnls, and a keep a couple of tidy-ups
for robustness.

vszakats added 12 commits April 10, 2026 16:10
```
configure:...: gcc -c -Werror-implicit-function-declaration -O2 -Wno-system-headers -D_GNU_SOURCE    conftest.c >&5
conftest.c:56:3: error: invalid preprocessing directive #default
   56 | # default includes
      |   ^~~~~~~
configure:...: $? = 1
configure: failed program was:
```
https://github.com/curl/curl/actions/runs/23639833978/job/68857514941?pr=21000
https://github.com/curl/curl/actions/runs/23644221708/job/68872229773?pr=21000
https://github.com/curl/curl/actions/runs/23644221703/job/68872228890?pr=21000

old linux:
Get:19 http://archive.debian.org/debian-archive/debian stretch/main amd64 autoconf all 2.69-10 [338 kB]
Get:20 http://archive.debian.org/debian-archive/debian stretch/main amd64 autotools-dev all 20161112.1 [73.4 kB]
Get:21 http://archive.debian.org/debian-archive/debian stretch/main amd64 automake all 1:1.15-6 [733 kB]

```
checking for nghttp2_session_get_stream_local_window_size in -lnghttp2... no
../configure: line 37874: syntax error near unexpected token `fi'
../configure: line 37874: `fi'
```

due to in ./configure:
```
if eval test \"x\$"$as_ac_Header"\" = x"yes"; then :
  cat >>confdefs.h <<_ACEOF
_ACEOF
 # to do if not found

else
  # to do if found

fi
```

later versions solve this by using `case`.
@vszakats vszakats changed the title configure: switch to # comments (from dnl) configure: tidy up comments Apr 10, 2026
@vszakats vszakats marked this pull request as ready for review April 10, 2026 14:59
@vszakats vszakats closed this in 8a3991e Apr 10, 2026
@vszakats vszakats deleted the am-comments branch April 10, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants