[llvm-bugs] [Bug 24401] New: Spacing issue with "UseTab=Always" option

[via llvm-bugs]

https://llvm.org/bugs/show_bug.cgi?id=24401

            Bug ID: 24401
           Summary: Spacing issue with "UseTab=Always" option
           Product: clang
           Version: trunk
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P
         Component: Formatter
          Assignee: unassignedclangbugs at nondot.org
          Reporter: cbeck at princeton.edu
                CC: djasper at google.com, klimek at google.com,
                    llvm-bugs at lists.llvm.org
    Classification: Unclassified

Created attachment 14713
  --> https://llvm.org/bugs/attachment.cgi?id=14713&action=edit
Patch which adds a unit test demonstrating the bug, @git revision
9d19c31c0b500c54d9276d4d8d0748a0820abe68

When using "UseTab=Always" together with "AlignTrailingComments", I get
misaligned trailing comments. The issue is fixed by using
"UseTab=ForIndentation".

I took an example from my project and coded it up as a clang unit test, patch
attached.

To make the issue concrete, here's what it looks like with
"UseTab=ForIndentation":

if (it != bytecode_cache_.end()) {
    lua_pushstring(L, bytecode_cache_key);     // key
    lua_rawget(L, LUA_REGISTRYINDEX);       // cache_table
    lua_rawgeti(L, -1, it->second);           // cache_table bytecode
    lua_remove(L, -2);               // bytecode
    lua_insert(L, -1 - nargs);           // bytecode [args x nargs]
    return protected_call(nargs);           //
} else if (load_string(script)) {           // bytecode
    lua_pushstring(L, bytecode_cache_key);     // bytecode key
    lua_rawget(L, LUA_REGISTRYINDEX);       // bytecode cache_table
    lua_pushvalue(L, -2);               // bytecode cache_table bytecode
    bytecode_cache_[script] = luaL_ref(L, -2); // bytecode cache_table
    lua_pop(L, 1);                   // bytecode
    lua_insert(L, -1 - nargs);           // bytecode [args x nargs]
    return protected_call(nargs);           //
} else {
    return false;
}

Here's what it looks like with "UseTab=Always"

if (it != bytecode_cache_.end()) {
    lua_pushstring(L, bytecode_cache_key);     // key
    lua_rawget(L, LUA_REGISTRYINDEX);      // cache_table
    lua_rawgeti(L, -1, it->second);           // cache_table bytecode
    lua_remove(L, -2);               // bytecode
    lua_insert(L, -1 - nargs);           // bytecode [args x nargs]
    return protected_call(nargs);           //
} else if (load_string(script)) {           // bytecode
    lua_pushstring(L, bytecode_cache_key);     // bytecode key
    lua_rawget(L, LUA_REGISTRYINDEX);      // bytecode cache_table
    lua_pushvalue(L, -2);               // bytecode cache_table bytecode
    bytecode_cache_[script] = luaL_ref(L, -2); // bytecode cache_table
    lua_pop(L, 1);                   // bytecode
    lua_insert(L, -1 - nargs);           // bytecode [args x nargs]
    return protected_call(nargs);           //
} else {
    return false;
}

The patch is a clang unit test (valid against master branch git commit
9d19c31c0b500c54d9276d4d8d0748a0820abe68) which tests that this is indeed the
current (incorrect) behavior, using style with minimal changes from LLVM style
which generates the behavior.

I tried to figure out how to fix it, I think that potentially the issue might
be a logic error here?

https://github.com/llvm-mirror/clang/blob/45aa4ad6e537866b40fe9574fa992bf3d6593e39/lib/Format/WhitespaceManager.cpp#L422

The code comment says "Indent with tabs only when there's at least one full
tab", and checks if the desired number of spaces is at least "FirstTabWidth"
(the width the first tab would have) + the width of a full tab. If so it emits
one tab character and subtracts that many spaces. Then we assume we are tab
aligned, and emit Spaces / TabWidth tabs, and Spaces % TabWidth spaces.

However, in the case that the tested condition fails, presumably we aren't
supposed to emit any tabs and should only emit spaces, but we will do so
anyways, at line 427.

I experimented by changing the code, to negate the condition and cause it to
simply emit "Spaces" many spaces and return when the condition has failed,
otherwise it follows the same code path as before. However, this breaks several
other unit tests. So I thought I should just submit what I've done as an issue
to the bugtracker instead.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20150808/a02ac2f4/attachment.html>