Clang keeps complain it ignores __declspec(dllexport) of basic_data<void> template instantitation definition in format.cc by denchat · Pull Request #2220 · fmtlib/fmt

denchat · 2021-04-08T18:57:36Z

Remove __declspec(dllexport) from the definition
and add template instantiation declaration (aka extern) in format.h

I'm not sure the added position is correct, please check if it's okey.

Clang keeps complain it ignores __declspec(dllexport) in format.cc's instantiation of "detail::basic_data<void>;" ``` C:/Users/User/AppData/Roaming/fmt-master/src/format.cc:58:17: warning: 'dllexport' attribute ignored on explicit instantiation definition [-Wignored-attributes] template struct FMT_INSTANTIATION_DEF_API detail::basic_data<void>; ^ C:/Users/User/AppData/Roaming/fmt-master/include\fmt/core.h:228:37: note: expanded from macro 'FMT_INSTANTIATION_DEF_API' # define FMT_INSTANTIATION_DEF_API FMT_API ^ C:/Users/User/AppData/Roaming/fmt-master/include\fmt/core.h:210:32: note: expanded from macro 'FMT_API' # define FMT_API __declspec(dllexport) ^ 1 warning generated. ``` I guess we have to make an explicit instantiation definition of `basic_data<void>` in format.cc (without `__declspec(dllexport)` ) and make an explicit instantiation declaration (aka `extern template`) in format.h instead

Add the template instantiation **declaration** of `basic_data<void>` the the template instantiation **definition** is in `format.cc`

denchat · 2021-04-08T19:12:23Z

Or should I move FMT_INSTANTIATION_DEF_API deletion into the WIN32 check branch (above current position) instead ?

vitaut

Could you post the full warning message that you get? Also there are some CI failures.

Try remove FMT_INSTANTIATION_DEF_API from extern template

Fix removing the entire declaration because at line 999 there is one. CI Error in macOS.

denchat · 2021-04-10T16:21:41Z

Could you post the full warning message that you get? Also there are some CI failures.

C:\Users\User\AppData\Roaming\fmt>cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_MAKE_PROGRAM="C:/Program Files (x86)/Ninja/ninja.exe" -DCMAKE_SYSTEM_NAME=Windows -DCMAKE_CXX_STANDARD=23 -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang++.exe" -DCMAKE_CXX_FLAGS="-fuse-ld=lld --target=x86_64-w64-windows-gnu --driver-mode=g++ -O3 -std=gnu++2b -static-libgcc -static-libstdc++ -fconstexpr-backtrace-limit=0 -lwinpthread -DNDEBUG -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -Wno-unused-command-line-argument" -DCMAKE_CXX_LINK_EXECUTABLE="C:/Program Files/LLVM/bin/lld.exe" -DBUILD_SHARED_LIBS=YES -DFMT_INSTALL=NO -DFMT_DOC=NO -DFMT_TEST=YES "C:/Users/User/AppData/Roaming/fmt-master"
-- CMake version: 3.20.0
-- The CXX compiler identification is Clang 12.0.4
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/LLVM/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Version: 7.1.3
-- Build type: Release
-- CXX_STANDARD: 23
-- Performing Test SUPPORTS_USER_DEFINED_LITERALS
-- Performing Test SUPPORTS_USER_DEFINED_LITERALS - Success
-- Performing Test FMT_HAS_VARIANT
-- Performing Test FMT_HAS_VARIANT - Success
-- Required features: cxx_variadic_templates
-- Performing Test HAS_NULLPTR_WARNING
-- Performing Test HAS_NULLPTR_WARNING - Success
-- Looking for _strtod_l
-- Looking for _strtod_l - found
-- Performing Test FMT_HAS_MBIG_OBJ
-- Performing Test FMT_HAS_MBIG_OBJ - Failed
-- Looking for C++ include pthread.h
-- Looking for C++ include pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Performing Test HAVE_FNO_DELETE_NULL_POINTER_CHECKS
-- Performing Test HAVE_FNO_DELETE_NULL_POINTER_CHECKS - Success
-- FMT_PEDANTIC: OFF
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/User/AppData/Roaming/fmt

C:\Users\User\AppData\Roaming\fmt>ninja -j 2
[3/51] Building CXX object CMakeFiles/fmt.dir/src/format.cc.obj
C:/Users/User/AppData/Roaming/fmt-master/src/format.cc:58:17: warning: 'dllexport' attribute ignored on explicit instantiation definition [-Wignored-attributes]
template struct FMT_INSTANTIATION_DEF_API detail::basic_data<void>;
                ^
C:/Users/User/AppData/Roaming/fmt-master/include\fmt/core.h:228:37: note: expanded from macro 'FMT_INSTANTIATION_DEF_API'
#  define FMT_INSTANTIATION_DEF_API FMT_API
                                    ^
C:/Users/User/AppData/Roaming/fmt-master/include\fmt/core.h:210:32: note: expanded from macro 'FMT_API'
#    define FMT_API __declspec(dllexport)
                               ^
1 warning generated.

By the way, this still does not succeed. It fails on compile-test.cc somehow.
#2223

However it's perhaps a clang 12 or mingw64 bug.

vitaut · 2021-04-11T03:26:03Z


 #if !defined(FMT_HEADER_ONLY) && defined(_WIN32)
 #  define FMT_CLASS_API FMT_MSC_WARNING(suppress : 4275)
+#  define FMT_INSTANTIATION_DEF_API


We cannot just remove __declspec(dllexport) because it will break a shared library. Instead it should be applied to a declaration that can be added to format.cc.

denchat · 2021-04-12T04:53:30Z

It seems there might has been some changes.

[MinGW] Fix dllexport of explicit template instantiation
https://reviews.llvm.org/D61118

To fix

Mark only at the extern template, not the definition
Restore behavior of FMT_INSTANTIATION_DEF_API to follow FMT_API

FMT_INSTANTIATION_DEF_API follows FMT_API

D61118 : make dllexport on extern template, not the instantiation definition in format.cc [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

Move FMT_INSTANTIATION_DEF_API to the declaration in format.h [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

denchat · 2021-04-12T05:11:44Z

More over, FMT_INSTANTIATION_DEF_API shall be considered to be renamed.
what about FMT_INSTANTIATION_DECL_API ?
Is it okey?

vitaut · 2021-04-12T13:35:35Z

More over, FMT_INSTANTIATION_DEF_API shall be considered to be renamed.
what about FMT_INSTANTIATION_DECL_API ?
Is it okey?

Sure

vitaut · 2021-04-12T14:00:50Z

 }  // namespace detail

-template struct FMT_INSTANTIATION_DEF_API detail::basic_data<void>;
+template struct detail::basic_data<void>;


Please add a short comment saying that clang doesn't allow __declspec(dllexport) here.

Rename FMT_INSTANTIATION_DEF_API to FMT_INSTANTIATION_DECL_API [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

Add a comment that clang now doesn't allow dllexport on template instantiation definitions. Mark them on tempalte instantiation declarations instead (in format.h). [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

denchat · 2021-04-12T15:20:10Z

A comment is added but please feel free to edit it.

vitaut

LGTM, just one remaining nit.

vitaut · 2021-04-12T15:32:30Z

 }  // namespace detail

-template struct FMT_INSTANTIATION_DEF_API detail::basic_data<void>;
+// Clang doesn't allow dllexport on template instantiation definitions (LLVM D61118)


nit: Please add . at the end of the sentence and apply clang-format.

vitaut · 2021-04-12T16:31:52Z

Thanks

denchat added 3 commits April 9, 2021 01:44

Add the template instantiation declaration

28882fa

Add the template instantiation **declaration** of `basic_data<void>` the the template instantiation **definition** is in `format.cc`

Fix incorrect namespace in previous commit

81aaff7

Move FMT_INSTANTIATION_DEF_API deletion into WIN32

70258cb

vitaut requested changes Apr 10, 2021

View reviewed changes

denchat added 2 commits April 10, 2021 23:04

Try remove FMT_INSTANTIATION_DEF_API

0e3c282

Try remove FMT_INSTANTIATION_DEF_API from extern template

Fix removing entire declaration

1bb8044

Fix removing the entire declaration because at line 999 there is one. CI Error in macOS.

vitaut requested changes Apr 11, 2021

View reviewed changes

denchat added 3 commits April 12, 2021 11:53

Restore the original behavior

c91cb64

FMT_INSTANTIATION_DEF_API follows FMT_API

D61118 : mark dllexport on extern template only

66f6f54

D61118 : make dllexport on extern template, not the instantiation definition in format.cc [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

Move FMT_INSTANTIATION_DEF_API to the declaration

9687bce

Move FMT_INSTANTIATION_DEF_API to the declaration in format.h [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

vitaut reviewed Apr 12, 2021

View reviewed changes

denchat added 3 commits April 12, 2021 22:15

Rename FMT_INSTANTIATION_DEF_API

8658826

Rename FMT_INSTANTIATION_DEF_API to FMT_INSTANTIATION_DECL_API [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

Rename FMT_INSTANTIATION_DECL_API

e2c1a2c

Rename FMT_INSTANTIATION_DEF_API to FMT_INSTANTIATION_DECL_API [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

Add a comment about clang

400a0a5

Add a comment that clang now doesn't allow dllexport on template instantiation definitions. Mark them on tempalte instantiation declarations instead (in format.h). [MinGW] Fix dllexport of explicit template instantiation https://reviews.llvm.org/D61118

vitaut reviewed Apr 12, 2021

View reviewed changes

do clang-formatting the comment

f0269f9

vitaut merged commit 42eccac into fmtlib:master Apr 12, 2021

vitaut mentioned this pull request Apr 12, 2021

warning C4910: '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation #2228

Closed

Navigation Menu

Uh oh!

Clang keeps complain it ignores __declspec(dllexport) of basic_data<void> template instantitation definition in format.cc - #2220

Clang keeps complain it ignores __declspec(dllexport) of basic_data<void> template instantitation definition in format.cc#2220
vitaut merged 13 commits into
fmtlib:masterfrom
denchat:patch-2

denchat commented Apr 8, 2021 •

edited

Loading

Uh oh!

denchat commented Apr 8, 2021

Uh oh!

vitaut left a comment

Uh oh!

denchat commented Apr 10, 2021

Uh oh!

vitaut Apr 11, 2021

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

vitaut commented Apr 12, 2021

Uh oh!

vitaut Apr 12, 2021

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

vitaut left a comment

Uh oh!

vitaut Apr 12, 2021

Uh oh!

vitaut commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

denchat commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denchat commented Apr 8, 2021

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

denchat commented Apr 10, 2021

Uh oh!

vitaut Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

vitaut commented Apr 12, 2021

Uh oh!

vitaut Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

denchat commented Apr 12, 2021

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

vitaut Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

vitaut commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

denchat commented Apr 8, 2021 •

edited

Loading