Skip to content

Rename StrCat to StringCat#544

Closed
winksaville wants to merge 1 commit intogoogle:masterfrom
winksaville:Rename-StrCat-to-StringCat
Closed

Rename StrCat to StringCat#544
winksaville wants to merge 1 commit intogoogle:masterfrom
winksaville:Rename-StrCat-to-StringCat

Conversation

@winksaville
Copy link
Copy Markdown
Contributor

StrCat is a macro in Shlwapi.h that conflicts with StrCat in string_util.h:

#defines StrCat lstrcatA

When Shlwapi.h is included in sysinfo.cc and all references to StrCat are
renamed to lstrcatA in string_util.h and sysinfo.cc, therefore this
renaming is innocuous, although unintended.

But if string_util.h is moved above Shlwapi.h then errors occur because
StrCat is not renamed in string_util.h but are renamed in sysinfo.cc:

ClCompile:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\bin\HostX86\x64\CL.exe /c /I"C:\Users\winks\prgs\google-benchmark\include" /I"C:\Users\winks\prgs\google-benchmark\src" /I"C:\Users\winks\prgs\google-benchmark\src..\include" /Zi /nologo /W4 /WX- /diagnostics:classic /Od /Ob0 /D WIN32 /D _WINDOWS /D _CRT_SECURE_NO_WARNINGS /D HAVE_STD_REGEX /D HAVE_STEADY_CLOCK /D "CMAKE_INTDIR="Debug"" /D _MBCS /Gm- /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /Fo"benchmark.dir\Debug\" /Fd"benchmark.dir\Debug\benchmark.pdb" /Gd /TP /errorReport:queue "C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc"
sysinfo.cc
C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc(228): error C2660: 'lstrcatA': function does not take 4 arguments [C:\Users\winks\prgs\google-benchmark\build\src\benchmark.vcxproj]
C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc(229): error C2664: 'LPSTR lstrcatA(LPSTR,LPCSTR)': cannot convert argument 1 from 'std::string' to 'LPSTR' [C:\Users\winks\prgs\google-benchmark\build\src\benchmark.vcxproj]
...
Done Building Project "C:\Users\winks\prgs\google-benchmark\build\test\basic_test.vcxproj" (default targets) -- FAILED.

This PR renames StrCat to StringCat in string_util.h, sysinfo.cc and
console_reporter.cc to fix this issue.

StrCat is a macro in Shlwapi.h that conflicts with StrCat in string_util.h:

   #defines StrCat lstrcatA

When Shlwapi.h is included in sysinfo.cc and all references to StrCat are
renamed to lstrcatA in string_util.h and sysinfo.cc, therefore this
renaming is innocuous, although unintended.

But if string_util.h is moved above Shlwapi.h then errors occur because
StrCat is not renamed in string_util.h but are renamed in sysinfo.cc:

  ClCompile:
    C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\bin\HostX86\x64\CL.exe /c /I"C:\Users\winks\prgs\google-benchmark\include" /I"C:\Users\winks\prgs\google-benchmark\src" /I"C:\Users\winks\prgs\google-benchmark\src\..\include" /Zi /nologo /W4 /WX- /diagnostics:classic /Od /Ob0 /D WIN32 /D _WINDOWS /D _CRT_SECURE_NO_WARNINGS /D HAVE_STD_REGEX /D HAVE_STEADY_CLOCK /D "CMAKE_INTDIR=\"Debug\"" /D _MBCS /Gm- /EHsc /RTC1 /MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR /Fo"benchmark.dir\Debug\\" /Fd"benchmark.dir\Debug\benchmark.pdb" /Gd /TP /errorReport:queue "C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc"
    sysinfo.cc
  C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc(228): error C2660: 'lstrcatA': function does not take 4 arguments [C:\Users\winks\prgs\google-benchmark\build\src\benchmark.vcxproj]
  C:\Users\winks\prgs\google-benchmark\src\sysinfo.cc(229): error C2664: 'LPSTR lstrcatA(LPSTR,LPCSTR)': cannot convert argument 1 from 'std::string' to 'LPSTR' [C:\Users\winks\prgs\google-benchmark\build\src\benchmark.vcxproj]
  ...
  Done Building Project "C:\Users\winks\prgs\google-benchmark\build\test\basic_test.vcxproj" (default targets) -- FAILED.

This PR renames StrCat to StringCat in string_util.h, sysinfo.cc and
console_reporter.cc to fix this issue.
@AppVeyorBot
Copy link
Copy Markdown

@pleroy
Copy link
Copy Markdown
Contributor

pleroy commented Mar 6, 2018

(FWIW I am using this library on Windows.)

This PR is moving in the wrong direction in my opinion as it would complicate the transition to Abseil, which I believe should be our (distant) goal as it would make it much easier to use Google libraries together.

The Windows headers are well known for polluting the macro space (it's the 90s, go for it!). If that gets in the way, just #undef the conflicting symbols (I'd vote for doing that just after the Windows includes for clarity). After all, this is sysinfo.cc, it already contains a lot of platform-specific crud.

@winksaville
Copy link
Copy Markdown
Contributor Author

@pleroy, if it’s decided that to use the undef solution, should we change other StringXxx names to StrXxx? With this change there is consistency, which seems a good thing.

@dmah42
Copy link
Copy Markdown
Member

dmah42 commented Mar 6, 2018

I agree that moving to absl is the right approach, which is dependent on the (multiple?) outstanding PRs to use bazel (or absl moving to support non-bazel build systems). However, until then, we shouldn't be blocking PRs that fix breakages.

Either the rename or the #undef are fine with me, though I do like the consistency argument of the rename (as a strictly short-term solution). @pleroy Is this ok with you too?

@pleroy
Copy link
Copy Markdown
Contributor

pleroy commented Mar 6, 2018

I'd rather undef, and rename String to Str if we care about consistency, as that seems to be what absl has chosen.

@dmah42
Copy link
Copy Markdown
Member

dmah42 commented Mar 6, 2018

SGTM, and moves us closer to absl.

@winksaville
Copy link
Copy Markdown
Contributor Author

Created PR #546 and #547, closing this PR.

@winksaville winksaville closed this Mar 6, 2018
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 87.048% when pulling d51efb1 on winksaville:Rename-StrCat-to-StringCat into 47df49e on google:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants