r195877 - strip UTF-8 BOM in -frewrite-includes (PR#15664)

[Lubos Lunak]

On Wednesday 27 of November 2013, Alp Toker wrote:
> On 27/11/2013 21:14, Lubos Lunak wrote:
> > Author: llunak
> > Date: Wed Nov 27 15:14:43 2013
> > New Revision: 195877
...
> >===== --- cfe/trunk/test/Frontend/Inputs/rewrite-includes-bom.h (added)
> > +++ cfe/trunk/test/Frontend/Inputs/rewrite-includes-bom.h Wed Nov 27
> > 15:14:43 2013 @@ -0,0 +1 @@
> > +// This file starts with UTF-8 BOM marker.
> >
> > Added: cfe/trunk/test/Frontend/rewrite-includes-bom.c
> > URL:
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/rewrite-inclu
> >des-bom.c?rev=195877&view=auto
> > =========================================================================
> >===== --- cfe/trunk/test/Frontend/rewrite-includes-bom.c (added)
> > +++ cfe/trunk/test/Frontend/rewrite-includes-bom.c Wed Nov 27 15:14:43
> > 2013 @@ -0,0 +1,4 @@
> > +// RUN: %clang -E -frewrite-includes -I %S/Inputs %s -o - | %clang
> > -fsyntax-only -Xclang -verify -x c - +// expected-no-diagnostics
> > +
> > +#include "rewrite-includes-bom.h"
>
> Hi Lubos,
>
> I don't think it's reasonable to check in this test in its current
> state, given that it passes with or without the BOM. If the invisible
> BOM happens to get removed by automatic encoding transformations in VCS
> or accidental code cleanup, it would give a false sense that the feature
> is working when it could in fact have regressed.

 Actually, most tests do no longer work if they have accidentally their input 
modified to no longer include whatever it is that should find a regression, 
and it's a question how likely it is to happen for any of them.

> Leaving the feature untested until you get round to writing a better
> test, perhaps checking output for presence of BOM with something like
> grep, is preferable for that reason.

 Now only if that were so easy:

> @@ -1,4 +1,5 @@
> -// RUN: %clang -E -frewrite-includes -I %S/Inputs %s -o - | %clang
> -fsyntax-only -Xclang -verify -x c - 
> +// RUN: grep '^\xEF\xBB\xBF'  %S/Inputs/rewrite-includes-bom.h
> +// RUN: %clang_cc1 -E -frewrite-includes -I %S/Inputs %s -o - | %clang_cc1
> -fsyntax-only -verify -x c - | not grep '\xEF\xBB\xBF'

 This test doesn't fail with the BOM removed either. With GNU grep 4.12 that I 
have here, the first grep never matches regardless of whether the BOM is 
there or not (which additionally means that even if the grep worked fine, the 
test still wouldn't fail for a regression).

 Also, the removal of the BOM is already handled by the -fsyntax-only step 
(which fails if the BOM is in the middle of the input), so the last grep 
always checks something where it's never present.

> Also, your commit email address is unreachable. Please update this so we
> can contact you for post-commit review:

 I already had asked for that before the commit. It's just been a long time 
since I last used Subversion.

-- 
 Lubos Lunak