llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.4k stars 11.74k forks source link

[clang-format] Pointer asterisk is formatted weirdly in certain cases #105898

Closed davidebeatrici closed 1 month ago

davidebeatrici commented 1 month ago

Before:

void HardHook::setupInterface(IUnknown *unkn, LONG funcoffset, voidFunc replacement) {
    fods("HardHook: setupInterface: Replacing %p function #%ld", unkn, funcoffset);
    void **ptr = reinterpret_cast< void ** >(unkn);
    ptr        = reinterpret_cast< void ** >(ptr[0]);
    setup(reinterpret_cast< voidFunc >(ptr[funcoffset]), replacement);
}

After:

void HardHook::setupInterface(IUnknown *unkn, LONG funcoffset, voidFunc replacement) {
    fods("HardHook: setupInterface: Replacing %p function #%ld", unkn, funcoffset);
    void **ptr = reinterpret_cast< void ** >(unkn);
    ptr        = reinterpret_cast< void        **>(ptr[0]);
    setup(reinterpret_cast< voidFunc >(ptr[funcoffset]), replacement);
}

Before:

            for (int i = 0; i < h; ++i) {
                const quint32 *srcimg  = reinterpret_cast< const quint32 * >(img.scanLine(i + h));
                const quint32 *srcmask = reinterpret_cast< const quint32 * >(img.scanLine(i));

                quint32 *dstimg  = reinterpret_cast< quint32 * >(out.scanLine(i));
                quint32 *dstmask = reinterpret_cast< quint32 * >(outmask.scanLine(i));

                for (int j = 0; j < w; ++j) {
                    dstmask[j] = srcmask[j];
                    dstimg[j]  = srcimg[j];
                }
            }

After:

            for (int i = 0; i < h; ++i) {
                const quint32 *srcimg  = reinterpret_cast< const quint32  *>(img.scanLine(i + h));
                const quint32 *srcmask = reinterpret_cast< const quint32 * >(img.scanLine(i));

                quint32 *dstimg  = reinterpret_cast< quint32  *>(out.scanLine(i));
                quint32 *dstmask = reinterpret_cast< quint32 * >(outmask.scanLine(i));

                for (int j = 0; j < w; ++j) {
                    dstmask[j] = srcmask[j];
                    dstimg[j]  = srcimg[j];
                }
            }

See my review in mumble-voip/mumble#6543.

davidebeatrici commented 1 month ago

Configuration:

Language:        Cpp
AccessModifierOffset: -4
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: true
# Setting AlignConsecutiveDeclarations to true would be nice but it doesn't work right with PointerAlignment=Right
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands:   true
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: InlineOnly
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: false
BinPackArguments: true
BinPackParameters: true
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeBraces: Attach
BreakBeforeTernaryOperators: true
BreakConstructorInitializers: BeforeColon
BreakStringLiterals: true
ColumnLimit:     120
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: false
DerivePointerAlignment: false
DisableFormat:   false
FixNamespaceComments: true
ForEachMacros:   
  - foreach
  - Q_FOREACH
  - BOOST_FOREACH
IncludeBlocks:   Preserve
IncludeCategories:
    # Global.h must always be included last
  - Regex: 'Global.h'
    Priority: 10000
    # Since a lot of windows-headers rely on windows.h, it has to be included first
  - Regex: 'windows.h'
    Priority: 1
    # Assign a priority < INT_MAX to all other includes in order to be able to force headers
    # to come after them
  - Regex: '.*'
    Priority: 10
IndentCaseLabels: true
IndentPPDirectives: AfterHash
IndentWidth:     4
IndentWrappedFunctionNames: true
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd:   ''
MaxEmptyLinesToKeep: 3
NamespaceIndentation: Inner
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 60
PointerAlignment: Right
ReflowComments:  true
SortIncludes:    true
SortUsingDeclarations: true
SpaceAfterCStyleCast: true
SpaceAfterTemplateKeyword: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles:  true
SpacesInContainerLiterals: true
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard:        Cpp11
TabWidth:        4
UseTab:          ForContinuationAndIndentation
owenca commented 1 month ago

Which version of clang-format gives you the "Before" code?

davidebeatrici commented 1 month ago

10

owenca commented 1 month ago

Bisected to 3e333cc82e42e1e2ecc974d896489eebe1a5edc2, which first appeared in 13.0.0.

davidebeatrici commented 1 month ago

Back in 2021... ouch. I'm surprised nobody else reported the bug in the meantime.

To be fair, I myself should've reported it earlier, but until now (bumping the tool version from 10 to 17 in a well established project) I had only encountered it a few times and in a somewhat regular fashion, leading me to think it could be a deliberate behavior change and the configuration needed to be fixed. Sorry about that.

In any case, thank you very much for fixing the bug so fast! Is there any chance for it to be backported?

owenca commented 1 month ago

I've added PR #106013 to the 19.X Release milestone.

owenca commented 1 month ago

Is there any chance for it to be backported?

The fix has been back ported to version 19.

davidebeatrici commented 1 week ago

Got it, thank you.