Thanks Dmitri,
I've fixed your comments.
About the test - I'm not exactly sure how to test this change - I'm not sure if 
committing a DOS-style file will work right. Any ideas?

Thanks
   Guy Benyei

-----Original Message-----
From: Dmitri Gribenko [mailto:[email protected]] 
Sent: Wednesday, February 06, 2013 15:44
To: Benyei, Guy
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] RE: [cfe-commits] r173697 - FileCheck'ize and merge tests

On Wed, Feb 6, 2013 at 3:00 PM, Benyei, Guy <[email protected]> wrote:
> Hi all,
> --strict-whitespace also kills the dos to unix end of line canonicalization. 
> This made 3 Clang LIT test fail on my Windows machine.
> Attached a patch that makes this flag affect the horizontal whitespaces only, 
> as it's stated in the flag's description.

This totally makes sense to me.  Nits:

1. Please add to the flag description in docs/CommandGuide/FileCheck.rst 
"End-of-line sequences are canonicalized to UNIX-style '\r' in all modes."

2. Please remove trailing whitespace from comments in your patch.

3.

+/// CanonicalizeInputFile - Remove duplicate horizontal space and dos 
+style /// '\r' at end of linefrom the specified memory buffer, free it, 
+and return /// a new one.

Please remove function name from the comment.  It would also help to rephrase 
it like:

"Canonicalize whitespace in the input file.  Line endings are replaced with 
UNIX-style '\r'.

\param PreserveHorizontal Don't squash consecutive horizontal whitespace 
characters into a single space."

4. Please add a test to test/FileCheck/

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Attachment: dos_eol_removal2.patch
Description: dos_eol_removal2.patch

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to