Re: [PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Corentin via cfe-commits]

I'll be unavailable the next 2 weeks, feel free to do it if you want!

On Fri, Jun 9, 2023, 21:10 Tom Honermann via Phabricator <
revi...@reviews.llvm.org> wrote:

> tahonermann added a comment.
>
> @cor3ntin, sorry for failing to keep up with reviews; I know this has
> already been committed. I did spot a couple of typos should you feel
> inclined to address them.
>
>
>
> 
> Comment at: clang/lib/Lex/Lexer.cpp:2695
> +  // diagnostic only once per entire ill-formed subsequence to avoid
> +  // emiting to many diagnostics (see
> http://unicode.org/review/pr-121.html).
> +  bool UnicodeDecodingAlreadyDiagnosed = false;
> 
>
>
>
> 
> Comment at: clang/lib/Lex/Lexer.cpp:2398
> +  // diagnostic only once per entire ill-formed subsequence to avoid
> +  // emiting to many diagnostics (see
> http://unicode.org/review/pr-121.html).
> +  bool UnicodeDecodingAlreadyDiagnosed = false;
> 
>
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D128059/new/
>
> https://reviews.llvm.org/D128059
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D136554: Implement CWG2631

[Corentin via cfe-commits]

Yes please! However the warning looks correct to me in that case. A
constructs x which constructs A etc.

On Mon, Jan 16, 2023, 18:00 Chris Bowler via Phabricator <
revi...@reviews.llvm.org> wrote:

> cebowleratibm added a comment.
>
> I've reduced a regression on:
>
> commit ca619613801233ef2def8c3cc7d311d5ed0033cb <
> https://reviews.llvm.org/rGca619613801233ef2def8c3cc7d311d5ed0033cb>
> (HEAD, refs/bisect/bad)
> Author: Corentin Jabot 
> Date:   Sun Oct 23 17:32:58 2022 +0200
>
>   template  int f(T) { return 42; }
>
>   struct A {
>  int x = f(A());
>  A() { }
>   };
>
>   void foo() { A(); }
>
>
>
>   clang++ t2.C -c
>   t2.C:4:12: warning: stack nearly exhausted; compilation time may suffer,
> and crashes due to stack overflow are likely [-Wstack-exhausted]
>  int x = f(A());
>  ^
>   Segmentation fault (core dumped)
>
> @cor3ntin would you like me to open a new issue?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D136554/new/
>
> https://reviews.llvm.org/D136554
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

[Corentin via cfe-commits]

My plan is to do that for all papers once past plenary to
keep consistency with cxx_status.

On Tue, Jul 5, 2022 at 6:13 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added reviewers: tahonermann, clang-language-wg.
> aaron.ballman added inline comments.
>
>
> 
> Comment at: clang/docs/ReleaseNotes.rst:271
> +- Added ``-Winvalid-utf8`` which diagnoses invalid UTF-8 code unit
> sequences in
> +  comments.
>
> 
> Should we mention `P2295R5` now that it's at least core approved?
> Something like:
> %%%
> ...unit sequences in comments; in support of `P2295R5 <
> http://wg21.link/P2295R5>`_.
> %%%
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D128059/new/
>
> https://reviews.llvm.org/D128059
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

[Corentin via cfe-commits]

On Thu, Apr 7, 2022 at 2:11 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D123298#3435796 , @cor3ntin
> wrote:
>
> > In D123298#3435770 ,
> @aaron.ballman wrote:
> >
> >> Changes LGTM, I also don't think we should hit these limits. Perhaps we
> should add some assertions to the ctor and the setter functions just to be
> sure though?
> >
> > If we are going to do anything, it ought to be a diagnostic?
>
> Doing a diagnostic would mean finding all the places where we form a
> `TemplateParmPosition` and ensure we have enough source location
> information to issue the diagnostic. Given that we don't expect users to
> ever hit it, having the assertion gives a wee bit of coverage (godbolt
> exposes an assertions build, for example) but without as much
> implementation burden. That said, if it's easy enough to give diagnostics,
> that's a more user-friendly approach if we think anyone would hit that
> limit. (I suspect template instantiation depth would be hit before bumping
> up against these limits, though.)
>

Fair enough - I suspect godbolt would die in that scenario though.
Note that i was not asking for a diagnostic, just saying that the assertion
may not protect anyone


>
> > I can't imagine a scenario in which someone would hit these limits and
> have assertions enabled. But i agree with you that the limit themselves
> should not be hit.
> > On the other hand, why not use 16 for both?
>
> I think people instantiate to deeper template depths than they typically
> have for template parameters, so having a deeper depth made sense to me.
>

Sure, but there is a huge imbalance there. 1048576 vs 4096 - I think it's
still better than msvc and it's conforming - the standard sets the minimum
at 1024 for both afaik


>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D123298/new/
>
> https://reviews.llvm.org/D123298
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D108742: [WIP] Reclassify form-feed and vertical tab as vertical WS for the purposes of lexing.

[Corentin via cfe-commits]

Thanks for the tip!

On Fri, Aug 27, 2021 at 9:31 PM Shivam Gupta via Phabricator <
revi...@reviews.llvm.org> wrote:

> xgupta added a comment.
>
> (If you pass --draft flag to arc while updating revision it becomes WIP so
> you don't need to retitle or remove reviewers. Buildbot also work in that
> case. And to again ask for review you can use `Add Action` drop down menu.)
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D108742/new/
>
> https://reviews.llvm.org/D108742
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D106577: [clang] Define __STDC_ISO_10646__

[Corentin via cfe-commits]

On Fri, Aug 13, 2021 at 7:42 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D106577#2943837 , @jyknight
> wrote:
>
> > In D106577#2904960 , @rsmith
> wrote:
> >
> >>> One specific example I'd like to be considered:
> >>> Suppose the C standard library implementation's mbstowcs converts a
> certain multi-byte character C to somewhere in the Unicode private use
> area, because Unicode version N doesn't have a corresponding character.
> Suppose further that the compiler is aware of Unicode version N+1, in which
> a character corresponding to C was added. Is an implementation formed by
> that combination of compiler and standard library, that defines
> `__STDC_ISO_10646__` to N+1, conforming? Or is it non-conforming because it
> represents character C as something other than the corresponding short name
> from Unicode version N+1?
> >>
> >> And David Keaton (long-time WG14 member and current convener) replied:
> >>
> >>> Yikes!  It does indeed sound like the library would affect the value
> of `__STDC_ISO_10646__` in that case.  Thanks for clarifying the details.
> >>
> >> There was no further discussion after that point, so I think the
> unofficial WG14 stance is that the compiler and the library need to collude
> on setting the value of that macro.
> >
> > I don't think that scenario is valid. MBCS-to-unicode mappings are a
> part of the definition of the MBCS (sometimes officially, sometimes
> de-facto defined by major vendors), not in the definition of Unicode.
>
> Isn't that scenario basically the one we're in today where the compiler is
> unaware of what mappings the library provides?
>
> > And in fact, we have a real-life example of this: the GB18030 encoding.
> That standard specifies 24 characters mappings to private-use-area unicode
> codepoints in the most recent version, GB18030-2005. (Which is down from 80
> PUA mappings in its predecessor encoding GBK, and 25 in GB18030-2000.) Yet,
> a new version of Unicode coming out will not affect that. Rather, I should
> say, DID NOT affect that -- all of those 24 characters mapped to PUAs in
> GB18030-2005 were actually assigned official unicode codepoints by 2005
> (some in Unicode 3.1, some in Unicode 4.1). But no matter -- GB18030 still
> maps those to PUA code-points. The only way that can change is if GB18030
> gets updated.
> >
> > I do note that some implementations (e.g. glibc) have taken it upon
> themselves to modify the official GB18030 character mapping table, and to
> decode those 24 codepoints to the newly-defined unicode characters, instead
> of the specified PUA codepoints. But there's no way that can be described
> as a requirement -- it's not even technically correct!
>
> Does that imply that an implementation supporting that encoding can't
> define __STDC_ISO_10646__ because it doesn't meet the "has the same value
> as the short identifier" requirement?
>

FYI, there should be a revision of GB18030 this year that will not use the
PUA anymore.
In general the PUA is considered "not for interchange" so if you have a
system that interprets PUA codepoints differently at different points in
time you are outside of any guarantees provided by Unicode.
GB18030-2005 is a weird exception as in general the standard library should
never transcode to the PUA as this is not portable.

GB18030, despite having a 1-1 mapping to unicode has to be considered a
distinct character set from Unicode, as such, a system where wide literals
are GB18030 encoded should not define
__STDC_ISO_10646__


>
> @jyknight, are you on the WG14 reflectors btw? Would you like to carry on
> with this discussion over there (or would you like me to convey your
> viewpoints on your behalf)?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D106577/new/
>
> https://reviews.llvm.org/D106577
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits