Skip to content

Conversation

@khwilliamson
Copy link
Contributor

Prior to this series of commits, this function was not getting compiled in some Configurations where it was needed, and not getting called always when needed.

* nothing on the more typical case where all possible categories present on
* the platform are handled. */
# ifdef HAS_IGNORED_LOCALE_CATEGORIES_
# if defined(HAS_IGNORED_LOCALE_CATEGORIES_) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point of information for a non-C programmer: Is there a difference between this syntax:

# ifdef SOMETHING

and this syntax:

# if defined(SOMETHING)

Why do you use one or the other?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef can only deal with one singe symbol
#if defined(FOO) is equal to #ifdef FOO, but when testing on more than one single symbol, like in this code, the second syntax is required
#if defined(FOO) || defined(BAR) does not have an #ifdef variant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef FOO is subsumed by the form #if defined FOO (or equivalently, #if defined ( FOO )). As for why both forms exist, I believe #ifdef (and #ifndef) came first historically and defined was added later when people needed more complex conditionals.

Choosing between them is mostly a matter of taste (personally I don't like the extra parentheses in the defined (FOO) form, for example). Some purists argue that you should limit yourself to #ifdef because anything more complex than that is a sign that you're abusing the preprocessor for things it was never meant to do (and you should presumably move complex configuration logic to a separate stage that emits simple macro definitions). In perl, that train has sailed.

So use 0 instead; 1 instead of 'true'
The next commit will want this header's information to be available for
perl_langinfo.h.
This will not compile if -DNO_LOCALE is specified, as well as
some other Configurations where various locale categories are missing or
confined to be the C locale.
@jkeenan
Copy link
Contributor

jkeenan commented Jan 30, 2024

As reported in #21909 (comment), the 3 commits in this pull request resolve the build-time failure reported in #21909 but generate 3 new build-time warnings. (The commit IDs below are from my local research branch.)

$ report-build-warnings 7d786c0271.linux.unthreaded.no-locale-messages.maketp.output.txt.gz 
File:  7d786c0271.linux.unthreaded.no-locale-messages.maketp.output.txt.gz

  Wunused-variable                           3

[make-output] 2020 $ parse-build-warnings 7d786c0271.linux.unthreaded.no-locale-messages.maketp.output.txt.gz 
File:  7d786c0271.linux.unthreaded.no-locale-messages.maketp.output.txt.gz

[
  {
    char   => 9,
    group  => "Wunused-variable",
    line   => 6504,
    source => "locale.c",
    text   => "unused variable \xE2\x80\x98localeconv_klen\xE2\x80\x99",
  },
  {
    char   => 18,
    group  => "Wunused-variable",
    line   => 6503,
    source => "locale.c",
    text   => "unused variable \xE2\x80\x98localeconv_key\xE2\x80\x99",
  },
  {
    char   => 28,
    group  => "Wunused-variable",
    line   => 6502,
    source => "locale.c",
    text   => "unused variable \xE2\x80\x98cat_index\xE2\x80\x99",
  },
]

From the output of make test_prep:

cc -c -DPERL_CORE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -
D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c99 -O2 -Wall -Werror=pointer-arith -Werror=vla -Wext
ra -Wno-long-long -Wno-declaration-after-statement -Wc++-compat -Wwrite-strings locale.c
locale.c: In function ‘S_emulate_langinfo’:
locale.c:6504:9: warning: unused variable ‘localeconv_klen’ [-Wunused-variable]
 6504 |     I32 localeconv_klen;
      |         ^~~~~~~~~~~~~~~
locale.c:6503:18: warning: unused variable ‘localeconv_key’ [-Wunused-variable]
 6503 |     const char * localeconv_key;
      |                  ^~~~~~~~~~~~~~
locale.c:6502:28: warning: unused variable ‘cat_index’ [-Wunused-variable]
 6502 |     locale_category_index  cat_index;
      |                            ^~~~~~~~~

So we should eliminate these new build-time warnings before merging this branch to blead. Once the warnings are gone, the p.r. should be reviewed to see if it meets its own objectives and if the C-level code is correct.

@khwilliamson
Copy link
Contributor Author

#21892 eliminates these warnings, in conjunction with this PR

@jkeenan
Copy link
Contributor

jkeenan commented Jan 30, 2024

#21892 eliminates these warnings, in conjunction with this PR

I continued with the QA procedure I described in #21909 (comment). I started with the investigative branch I described in that ticket and merged #21892 into it. I then configured and built twice, first with my regular (Linux) configuration switches, then adding -Accflags=-DNO_LOCALE_MESSAGES. In both cases, make test_prep was successful and emitted no warnings. I also ran the second case through make test_harness and got PASS.

Since we've been juggling quite a few locale-related pull requests lately, I recommend we get final review on this p.r. and #21892 (i.e., review by someone other than me), merge them into blead, and only then look to review additional p.r.s.

@khwilliamson khwilliamson merged commit 7fa3c53 into Perl:blead Feb 5, 2024
@khwilliamson khwilliamson deleted the compile_emulate branch February 5, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants