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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
