[PATCH] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:2544
+defm diagnostics_show_line_numbers : BoolFOption<"diagnostics-show-line-numbers",
+  DiagnosticOpts<"ShowLineNumbers">, DefaultTrue,
+  NegFlag<SetFalse, [CC1Option], "Show line numbers in diagnostic code snippest">,
----------------
aaron.ballman wrote:
> I'm hopeful that defaulting this to true (which matches the GCC behavior) is reasonable, but I'm a bit worried about how downstreams will react to this. Specifically, I'm wondering about things like clang-cl integration in MSVC or clang use in Xcode, where diagnostics are being displayed other than on the command line.
I don't like line number margins that much, but I think it should be Ok. What I've seen of compiler output parsers, they mainly concentrate on the `<file>:<line>[:<column>]: <kind>: <message>` lines and ignore code snippets. After all, once they have the source location, they can just look at the file.


================
Comment at: clang/lib/Frontend/TextDiagnostic.cpp:1121-1138
+static unsigned getNumDisplayWidth(unsigned N) {
+  if (N < 10)
+    return 1;
+  if (N < 100)
+    return 2;
+  if (N < 1'000)
+    return 3;
----------------
efriedma wrote:
> aaron.ballman wrote:
> > jrtc27 wrote:
> > > kwk wrote:
> > > > This function screamed at me to be generalized so I gave it a try: https://gist.github.com/kwk/7e408065ea291e49fea4a83cf90a9cdf
> > > I don't think I want floating point arithmetic in my compiler... Something like:
> > > 
> > > ```
> > >     unsigned L, M;
> > >     for (L = 1U, M = 10U; N >= M && M != ~0U; ++L)
> > >         M = (M > ((~0U) / 10U)) ? (~0U) : (M * 10U);
> > >     
> > >     return (L);
> > > ```
> > > 
> > > is the generalised form (without all the redundant parentheses I added during debugging).
> > Cleaned up a bit:
> > ```
> > constexpr unsigned getNumDisplayWidth(unsigned N) {
> >   unsigned L = 1U, M = 10U;
> >   constexpr unsigned Upper = ~0U / 10U;
> >   for (; N >= M && M != ~0U; ++L)
> >     M = M > Upper ? ~0U : M * 10U;
> >   return L;
> > }
> > ```
> > https://godbolt.org/z/szTYsEM4v
> Slightly less efficient, but easier to read:
> 
> ```
> unsigned getNumDisplayWidth(unsigned N) {
>   unsigned Width = 1;
>   for (; N >= 10; ++Width)
>     N /= 10;
>   return Width;
> }
> ```
I'd suggest to replace `~0U` by `std::numeric_limits<unsigned>::max()`. And if we're looking at `<limits>`, why not ask it directly how far we need to go?
```
static unsigned getNumDisplayWidth(unsigned N) {
  unsigned L = 1u, M = 10u;
  constexpr unsigned Max = std::numeric_limits<unsigned>::digits10 + 1;
  for (; M <= N && L != Max; ++L)
    M *= 10u;
  return L;
}
```
This will let the overflow happen, but that should be fine for unsigned arithmetic. Without overflow:
```
static unsigned getNumDisplayWidth(unsigned N) {
  unsigned L = 1u, M = 10u;
  while (M <= N && L++ != std::numeric_limits<unsigned>::digits10)
    M *= 10u;
  return L;
}
```
The trick is that the increment is done even if the comparison fails and we exit the loop.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147875/new/

https://reviews.llvm.org/D147875