Page MenuHomePhabricator

[ADT][Support] Fix C4146 error from MSVC
ClosedPublic

Authored by vinograd47 on Jan 11 2021, 7:24 AM.

Details

Summary

Unary minus operator applied to unsigned type, result still unsigned.

Use ~0U instead of -1U and 1 + ~VAL instead of -VAL.

Diff Detail

Unit TestsFailed

TimeTest
150 msx64 windows > LLVM.tools/dsymutil/ARM::extern-alias.test
Script: -- : 'RUN: at line 38'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\dsymutil.exe -oso-prepend-path C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM/../Inputs C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM/../Inputs/private/tmp/private_extern/private_extern.out -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\tools\dsymutil\ARM\Output\extern-alias.test.tmp.dSYM --verbose 2>&1 | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\tools\dsymutil\ARM\extern-alias.test

Event Timeline

vinograd47 created this revision.Jan 11 2021, 7:24 AM
vinograd47 requested review of this revision.Jan 11 2021, 7:24 AM

Perhaps we could rewrite the code to use ~ or the like instead of unary - if that works?

RKSimon added a subscriber: RKSimon.

Perhaps we could rewrite the code to use ~ or the like instead of unary - if that works?

+1 We currently globally disable C4146 inside llvm-project\llvm\cmake\modules\HandleLLVMOptions.cmake (along with a lot of other warnings that ideally we'd enable and make use of) - it'd be great if you could fix at least the code here instead of adding a lot of pragmas.

vinograd47 retitled this revision from [ADT][Support] Disable C4146 MSVC warning to [ADT][Support] Fix C4146 error from MSVC.
vinograd47 edited the summary of this revision. (Show Details)

Reworked the patch. Fixed the warning instead of disabling it.

dblaikie accepted this revision.Jan 12 2021, 11:26 AM

Seems OK - I don't know if there's a nicer way to handle the latter cases that need the 1 + ~N.

This revision is now accepted and ready to land.Jan 12 2021, 11:26 AM

Update the revision using Arcanist tool

Can you commit this yourself (do you have commit access), or do you need someone to commit it for you?

@dblaikie I'm not sure if I have commit access. But after rebase CI shows unit tests failures, I need to investigate them prior to merging.

Rebased to the latest main state.

@dblaikie I see similar failures locally even on clean main branch without this changes. Is it some known issue?

@dblaikie CI now passed. Could you please commit this change to main repo?

This revision was automatically updated to reflect the committed changes.