On 8 October 2020 23:39:15 CEST, JeanHeyd Meneide via Gcc-patches 
<gcc-patches@gcc.gnu.org> wrote:
>Dear Joseph,
>
>On Thu, Oct 8, 2020 at 1:36 PM Joseph Myers <jos...@codesourcery.com>
>wrote:
>>
>> This documentation doesn't seem sufficient to use the macros.  Do
>they
>> expand to (narrow) string literals?  To an unquoted sequence of
>> characters?  I think from the implementation that the answer is
>strings
>> (so, in particular, not usable for testing anything in #if
>conditionals),
>> but the documentation ought to say so.  The test ought to verify the
>form
>> of the expansion as well (even if it can't do anything useful at
>execution
>> time, because if you make the macros reflect the command-line options
>they
>> are character set names that are meaningful on the host, and any
>> conversion functionality on the target may not use the same names as
>the
>> host).
>
>     You're right; sorry about that, I should have been more thorough!
>I thought about adding a test to check the name itself (e.g, for
>"UTF-8"), but that might make tests fail on platforms where the
>default SOURCE_CHARSET from the dev files is not, in fact, UTF-8. I
>could also try to pass some options but then I'd have to guarantee
>that the encoding was available on all testable platforms, too...!
>
>    In the end, for the tests, I just initialize two "const char[]"
>directly from the macro expansions to make sure we are getting
>strings. It seeeeeems to work okay. Attached is the revised patch with
>better docs and test!

Typo:  comple-time

>2020-10-08  JeanHeyd "ThePhD" Meneide  <phdoftheho...@gmail.com>
>
>        * gcc/c-family/c-cppbuiltin.c: Add predefined macro
>definitions for charsets

I think you should put the macro names in braces after the filename and drop 
the trailing "for charsets".

>        * gcc/doc/cpp.texi: Document new predefined macro.
>    * gcc/testsuite/c-c++-common/cpp/wide-narrow-predef-macros.c (new):

I think you should drop "(new)" above.
thanks,

>          New test for macro definitions to always exist.
>        * libcpp/include/cpplib.h: Add functions declarations for
>          retrieving charset names
>    * libcpp/directives.c: Add function definitions to retrieve charset
>          names.
>        * libcpp/internal.h: Add to/from name preservations

Reply via email to