klimek added inline comments.

================
Comment at: lib/Format/WhitespaceManager.cpp:678
     // Indent with tabs only when there's at least one full tab.
-    if (FirstTabWidth + Style.TabWidth <= Spaces) {
+    if (Style.TabWidth <= Spaces) {
       Spaces -= FirstTabWidth;
----------------
guiguiiiiiiii wrote:
> klimek wrote:
> > Why is this not just if (FirstTabWidth <= Spaces) then?
> Actually, that was my first way of fixing it. 
> 
> ```
> if (FirstTabWidth <= Spaces && Spaces > 1) // to avoid converting a single 
> space to a tab
> ```
> 
> Doing so will produce a correct output but introduce what I thought could be 
> an unwanted change : whitespace less than **TabWidth** might get converted to 
> a tab.
> The comment above the test seems to indicate that this behavior was not 
> originally wanted, that's why I changed my mind. 
> 
> But after thinking it over, I think my fix attempt also introduce an unwanted 
> change. I misinterpreted the original goal of this option.
> I went back to the official documentation and it says
> 
> > Use tabs whenever we need to fill whitespace that spans at least from one 
> > tab stop to the next one
> 
> This is clearly not what this code is doing. I can land another patch for a 
> more appropriate fix but I wonder if this option should stay like this. 
> Aren't people expecting the formatting to use as much tab as possible with 
> UT_Always ?
> 
> Unless I'm mistaken, the following example (**TabWidth** of 4)
> 
> 
> ```
> int a = 42;
> int aa = 42;
> int aaa = 42;
> int aaaa = 42;
> ```
> would become (following what's written in the documentation)
> 
> ```
> int a    = 42;
> int aa   = 42;
> int aaa  = 42;
> int aaaa = 42; 
> // only spaces since there's never enough whitespace to span a full tab stop
> ```
> And this is what I would expect:
> 
> ```
> int a  = 42; // int a\t = 42;
> int aa         = 42; // int aa\t = 42;
> int aaa        = 42; // int aaa\t = 42;
> int aaaa = 42; // int aaaa = 42;
> ```
The intent of the Always setting was to mirror what the Linux kernel wants, so 
whatever that is, we should probably follow it :)


Repository:
  rC Clang

https://reviews.llvm.org/D48259



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to