[PATCH] D15077: [PATCH] Save and restore current term setting in terminalHasColors(). PR25007

[Joerg Sonnenberger via llvm-commits]

joerg added a comment.

In http://reviews.llvm.org/D15077#301348, @mitchnull wrote:

> In http://reviews.llvm.org/D15077#300234, @joerg wrote:
>
> > I see two issues with the current code and I think they should be considered
> >  as a whole for any chance here.
> >
> > (1) We should only ever run this dance once. Even with clang we currently run it per source input.
>
>
> I don't think it's such a big issue, most build systems compile a  single file at a time,
>  not multiple source files on the same command line. I think it's better to keep the
>  most frequent use-case simple (and fast).


There have been long discussions about things like clangd and other uses where
it is done much more than once. That isn't very relevant, the point remains that we do
completely unnecessary recomputations, which are not that cheap either. Implementing
it doesn't complicate code by more than a few lines either.

> > (2) Since the dance is inherently not thread-safe, code using it as library,

> 

> > 

> 

> >   directly or indirectly, should have a way to either provide a fixed option

> 

> >   OR to initialize the state at a safe point.

> 

> 

> I glanced at the gnu ncurses code, and apparently it does have some

>  thread-safety (at least cur_term is protected in set_curterm).  Not sure

>  about other curses implementations though.  Still, this patch doesn't

>  make anything worse, imo.


It might protect its internal data structures by atomic updates. That doesn't
mean that a console editor using clang libraries in one thread wants its output
messed up from another thread because the terminal settings just changed.
They both depend on the global state of terminfo after all.


http://reviews.llvm.org/D15077