Conflict between _T macro and Sun Studio 12 headers

[jroemmler]

Hi,
I'm compiling and using wxWidgets library in various versions and platforms on a number of different machines. Lately, I came across a serious problem with macro _T which is defined in wxWidgets header files (see  http://docs.wxwidgets.org/stable/wx_stringfunctions.html#underscoret). A simple "program" like this won't compile any longer:

// file bad.cpp
#include <wx/wx.h>

# /opt/SUNWspro/bin/CC -V
CC: Sun C++ 5.8 Patch 121018-18 2009/02/25
# /opt/SUNWspro/bin/CC -I/usr/local/wxWidgets-2.6.3/build_Motif_CC_shared/lib/wx/include/motif-ansi-release-2.6 -I/usr/local/wxWidgets-2.6.3/include -I/usr/local/wxWidgets-2.6.3/contrib/include -D__WXMOTIF__ -I/usr/openwin/include -I/usr/dt/include -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -xarch=amd64 -o /dev/null -c bad.cpp
"/opt/SUNWspro/prod/include/CC/Cstd/./memory", line 107: Error: Type name expected instead of "{".
"/opt/SUNWspro/prod/include/CC/Cstd/./string", line 850:     Where: While specializing "__rwstd::__rw_basis<__rwstd::_T, __rwstd::_Base>".
"/opt/SUNWspro/prod/include/CC/Cstd/./string", line 850:     Where: Specialized in std::string .
"/opt/SUNWspro/prod/include/CC/Cstd/./stdexcept", line 65:     Where: Specialized in non-template code.

The problem is because _T is also used in template code by latest Sun Studio (11 and 12) STL header files (see my posting here:  http://forums.sun.com/thread.jspa?threadID=5376645). I think using names with underscore-prefix in (public) header files is definitely a bad idea.

My suggestion for wxWidgets 3.0 would be to break source code compatibility and get rid of _T() (and _() while we're at it); using existing wxT() instead is a simple search'n'replace operation - or making it a #define like so:

#ifdef wxUSE_DEPRECATED_T_MACRO
#define _T ...
#else
// do not define _T because it may lead to name clashes in user code
// or STL compiler header files.
#endif

What do you think?
Jochen

[troelsk]

I think using names with underscore-prefix in (public) header files is definitely a bad idea

Indeed. _T is a Windows-ism. Underscore-prefix is used by compiler vendors. wxWidgets should stick to the wx-prefix and lose _T altogether.

[jroemmler]

Thank you troelsk. I already thought that Windows is the "culprit" here...
Anybody else want to comment this?
This could also be fixed backward compatibly in STABLE branch (2.8.x) by
defining proposed wxUSE_DEPRECATED_T_MACRO there by default...
(and not defining it by default in 3.0?)

[vadz]

Sorry, it's simply impossible to break backwards compatibility like this, there are tons of code using this macro. And while I do know that identifiers starting with underscore are reserved for implementation the choice of _T is still very poor because it is a de facto standard macro under Windows and so I'd expect other code than wx to use it in the same way too.

Luckily, work around is trivially simple: #undef _T after including wx headers. Maybe we could allow to predefine a wxNO__T_MACRO to prevent it from being defined in the first place (patches welcome). But it definitely should be continued to be defined by default.

P.S. And this has nothing to do with _() at all AFAICS.

[jroemmler]

Vadim,

(1) Breaking backward compatibility is not necessary as I pointed out in my last posting (it depends on whether you #define WX_NO_T_MACRO by default or not).

(2) Your suggested work around won't do (see my first message): the name clash is caused by code within wxWidgets header files themselves. As a user, I simply have no chance to avoid the clash.

(3) Being a "cross-platform" GUI framework, you shouldn't ignore the world outside MSW (where _T is no de facto standard). IMHO losing support for the whole Sun Studio compiler suits would be a great loss (this compiler is quite popular, and it's free after all).

If you still decide to leave this ticket closed, it's your choice.

[troelsk]

It makes sense to do a 'soft' breaking of backward compatibility by deprecating _T.

[jroemmler]

Deprecating _T would also have my vote.

[vadz]

Before we get another hundred of votes for this, please notice that macros can't be deprecated.

Anyhow, I didn't realize that just including wx/wx.h triggered the problem, it's true that we must do something to fix this. AFAICS wxNO__T_MACRO -- to be predefined at compiler command line -- is still the simplest solution. It should be relatively trivial to do this, patches still welcome (to anybody submitting one: please make sure to document the new macro).

An alternative could be to undefine _T in wx/beforestd.h and redefine it again in wx/afterstd.h (possibly only for Sun compiler). I don't have time to download and install latest Sun CC and test this now however.

[jroemmler]

OK, just got an INTERNAL ERROR from your new ticket system while submitting, so I'm writing again....

Vadim, it is not necessary to become upset.
Just wanted to point your attention to the fact that wxWidgets can't compile with latest Sun Studio (any more). According to a Sun engineer, using names starting with underscore are reserved for internal use (I personally don't know if this statement is actually part of any C++ standard though). If it is common for MSW to define _T it's fine - but it doesn't make it a standard just because of that. Nobody knows if other compiler vendors choose to use _ or _T as external names in the future.

Regarding your suggestions to fix this, I like both ideas. Unfortunately, I don't have enough wxWidgets knowledge and time to submit a full patch, but I'm willing to compile code using Sun CC if something becomes available.

BTW: macros can be deprecated:
(1) in the docs (OK probably less efficient)
(2) at compile time (at least on Windows with VS2005 and option /W3)

// t.C
#pragma deprecated(_T)
const char* str = _T("hello world");

C:\> cl -nologo -c -Tpt.C /W3
t.C
t.C(4) : warning C4995: '_T': name was marked as #pragma deprecated

[vadz]

Replying to jroemmler:

OK, just got an INTERNAL ERROR from your new ticket system while submitting, so I'm writing again....

This is indeed painful but unfortunately there is simply not much we can do about this. Trac is nice to use when it works but it seems to be a resource hog and so it regularly breaks down :-(

Just wanted to point your attention to the fact that wxWidgets can't compile with latest Sun Studio (any more).

Again, I do agree that this is a problem, I thought it could be worked around easily but if it can't, then we need to do something about it.

According to a Sun engineer, using names starting with underscore are reserved for internal use (I personally don't know if this statement is actually part of any C++ standard though).

Yes, it is. They're perfectly in their right to use _T formally, it's just not a great choice in practice.

If it is common for MSW to define _T

Yes it is defined in standard MSVC tchar.h header (as well as _TEXT and __T).

Nobody knows if other compiler vendors choose to use _ or _T as external names in the future.

_ is a gettext standard so one can reasonably hope that nobody is going to use this, at least.

BTW: macros can be deprecated:
(1) in the docs (OK probably less efficient)
(2) at compile time (at least on Windows with VS2005 and option /W3)

I didn't know that this worked to be honest; this still definitely doesn't work with gcc (which uses an attribute and not a pragma for deprecation). In any case, deprecating _T is not realistic, just do a Google code search for the number of times it is used.

[lanurmi]

Replying to vadz:

Replying to jroemmler:

Nobody knows if other compiler vendors choose to use _ or _T as external names in the future.

_ is a gettext standard so one can reasonably hope that nobody is going to use this, at least.

It is certainly a common practice to use '_' in code, but it is far from standard, as GNU gettext itself does not define a function nor a macro with such a name, as far as I know.

[troelsk]

Eliminating _T entirely from wxWidgets is the better, and simpler, solution. It was a mistake to ever introduce it, not made better by keeping it. Better stick with the wx 'namespace' and only this.

[troelsk]

Didn't mean to close, sorry, just happened.

[lanurmi]

There would be a good chance to correct this mistake now, before 3.0, which is likely to break some existing code anyway. The transition from _T to wxT in end-user code is a matter of search-and-replace, and can be done already while still using 2.8 or 2.6 or 2.anything.

[vadz]

I really don't think it's a good idea to require people to run a global search and replace in their code when upgrading to 3.0, this is not going to facilitate its adoption. We can have wxNOT mentioned before and we could probably even set it automatically when using Sun compiler but we don't want to do this by default (as most of the statistics this is a wild guess but I'd expect the number of people using _T() in their code to be greater than number of people using wx with Sun compiler by several orders of magnitude).

[BrianCameron]

Wouldn't a simpler (and less invasive) fix just be to modify the wxWidgets header files to use "wxT" everywhere, and then simply define "_T" to be "wxT"? Then the definition of "_T" could be surrounded by "#ifndef SUNPRO_CC" so that it isn't available when using the Sun Studio compiler?

This way, programs would compile on Solaris as long as they use wxT. Programs that use _T would need to be patched to use wxT instead if users wanted to compile them on Solaris with Sun Studio. This seems a reasonable compromise. Though if you wanted to further deprecate _T, that would also be fine.

I would be happy to put together a patch that did this if this were an acceptable approach.

[vadz]

Yes, this is exactly what I suggested: my wxNO__T could be set to 1/defined for Sun CC automatically and _T defined only if it is true/defined. Changing all occurrences of _T in the headers is a time-consuming but simple task.

[wittyman]

As suggested in one of the comments earlier, I have tried the following patch and it seems to work well on Solaris (with new SS12).

<snip>
diff -cr wxGTK-2.8.9-orig3/include/wx/afterstd.h wxGTK-2.8.9-new3/include/wx/afterstd.h
*** wxGTK-2.8.9-orig3/include/wx/afterstd.h Thu May 14 22:58:23 2009
--- wxGTK-2.8.9-new3/include/wx/afterstd.h Thu May 14 23:21:41 2009
***************
*** 46,48 ****
--- 46,59 ----

#endif

#endif

+ #if defined( SUNCC )
+ #ifndef _T
+ #if !wxUSE_UNICODE
+ #define _T(x) x
+ #else /* Unicode */
+ /* use wxCONCAT_HELPER so that x could be expanded if it's a macro */
+ #define _T(x) wxCONCAT_HELPER(L, x)
+ #endif /* ASCII/Unicode */
+ #endif /* !defined(_T) */
+
+ #endif /* SUNCC */
diff -cr wxGTK-2.8.9-orig3/include/wx/beforestd.h wxGTK-2.8.9-new3/include/wx/beforestd.h
*** wxGTK-2.8.9-orig3/include/wx/beforestd.h Thu May 14 22:58:23 2009
--- wxGTK-2.8.9-new3/include/wx/beforestd.h Thu May 14 23:10:08 2009
***************
*** 63,65 ****
--- 63,68 ----

#pragma warning(disable:4786)

#endif // VC++ < 7

+ #if defined(SUNCC)
+ #undef _T
+ #endif // SUNCC
</snip>

[vadz]

I'm perfectly fine with applying this patch but notice that it won't solve the problem if you include a standard header after including wx/wx.h in your own program -- unless you use wx/beforestd.h and wx/afterstd.h too but this would be painful. So maybe it's still worth to not define _T() at all for Sun CC when #ifndef WXBUILDING.

Also, the patch is completely mangled, could you please attach it or at least use triple curly braces around it if you paste it in (telling Trac to keep it intact)?

Thanks!

[BrianCameron]

I have a patch to fix this problem in a better way, as proposed earlier. This patch modifies include/wx/wxchar.h
to define the wxT macro first, and then define _T to equal wxT as follows:

#if !defined (SUNPRO_C) && !defined(SUNPRO_CC)
#ifndef _T
#define _T(x) wxT(x)
#endif
#endif

This prevents the _T macro from being defined with the Sun Studio compiler. This allows wxWidgets to compile with Sun Studio fine, and allows programs which use wxWidgets to compile as long as the wxT macro is used instead of _T. It should be possible to easily patch programs to just use wxT and get such patches upstream if necessary.

The patch is large since there are a lot of occurances of _T being used in the wxWidgets code. You can access the patch here:

 http://yippi.hypermall.com/wxwidgets-02-Tmacro.diff

Can this go upstream? I think this is a better approach than the previous patch. It also makes the wxWidgets code more consistent. Now it just always uses wxT instead of the current mix of _T and wxT.

[vadz]

Thanks for the patch Brian, I tried to look at this as soon as possible but unfortunately it doesn't apply already:

% patch -p0<wxwidgets-02-Tmacro.diff --dry-run --quiet
1 out of 2 hunks FAILED -- saving rejects to file include/wx/version.h.rej
1 out of 25 hunks FAILED -- saving rejects to file src/common/filename.cpp.rej
6 out of 7 hunks FAILED -- saving rejects to file src/common/prntbase.cpp.rej
3 out of 5 hunks FAILED -- saving rejects to file src/richtext/richtextfontpage.cpp.rej

Also, the patch is so large as to be effectively unreviewable which is a bit worrisome. It probably could exclude all mac and msw files (which are never going to be compiled with Sun CC) but it wouldn't make it small enough probably. And, to address "as proposed earlier" part: I thought we'd only change the occurrences of _T in the _headers_, not everywhere. I think it combined with the patch of comment:17 should be enough to make wx itself compile as it already uses wx/beforestd.h and wx/afterstd.h around all standard headers it includes. And this would make for a much smaller patch.

Concerning the main part of the patch, i.e. changes to wx/wxchar.h, I'd still prefer to have wxUSE__T_MACRO or something like this which would be set automatically to 0 if __SUNPRO_CC is defined and 1 if it isn't but which could be pre-defined manually too: this will allow people to ensure that their code compiles without _T (and, to a lesser extent, also make it possible for someone to compile a program using a lot of _Ts with Sun compiler if something like patch from comment:17 is applied).

Finally, we really need to patch both 2.8 and HEAD if we do this.

So I still think that my suggestion in comment:18 is the least intrusive and hence simplest way to fix it in 2.8. For the HEAD we could apply your patch although I still think changing it in the headers only would be better.

Thanks!

[BrianCameron]

Thanks for reviewing my patch. Still, I think it is good cleanup to make the code consistent and to use the same macro everywhere, rather than the current mish-mash. I have gone ahead and created patches that apply to the 2.8 branch and the head branch as of today. See here:

 http://yippi.hypermall.com/wxw.tar.bz2

The tarball contains two patches wxwidgets-2.8.diff and wxwidgets-head.diff. It also contains a simple shell script that I use to verify that I didn't accidentally change any unintended lines. It basically does some grepping on the lines in the patch that start with "+" or "-" to verify that only the "_T(" macro was removed and only the "wxT(" macro is added. I hope this helps you to review the patch and feel more confident that the patch isn't messing up anything.

Although I'm sure that "_T(" macros will creep back in the code over time, I think cleaning this up now will obviously making keeping things cleaner in the future more easy. So I think the patch against head makes a lot of sense to apply.

I agree that combining this patch with the patch in comment #17 is a good idea, just to avoid problems if "_T(" macros do creep back into the codebase. Though if you don't apply that patch in comment #17, then you know that the folks at Sun will continue to provide patches to change any _T macros to wxT, and thus keep the code cleaner. Either way, I don't think it is a big deal.

I also agree that adding a wxUSET_MACRO is a good idea, though not strictly necessary. I'm not sure how to do that, but it sounds like it probably isn't too hard to add for someone who knows how to make that sort of change.

If you want to avoid making changes to directories like mac and msw in the 2.8 branch that seems reasonable. I can regenerate the patch if you like - just let me know which directories to remove from the patch, or you can simply apply the patch and remove the patched files from the directories you think should not be changed in the 2.8 branch before committing.

[vadz]

Thanks for the update Brian, I've applied the patch to the svn trunk in r61508.

I'm still extremely reluctant to apply such huge patch to 2.8, especially considering that it touches tons of code which is very rarely compiled and I'd much prefer the most minimalistic approach possible for 2.8.

So I think I'm finally going to apply patch from comment:17 but it would be really great if someone actually using the latest Sun CC could retest it and attach it as a patch so it could be applied automatically instead of doing it manually and risking the mistakes.

Thanks again!

[BrianCameron]

updated patch based on comment #18

[BrianCameron]

I can verify that the patch I just attached, which is based on comment #18 fixes the build problems.

I have also created a patch at  http://yippi.hypermall.com/wxw-2.8-smaller.diff which modifies the "_T(" macro to the "wxT(" macro for only those directories needed to build on Solaris. It is considerably smaller than the previous patch.

It might be good to apply both.

I also have created a patch at  http://yippi.hypermall.com/wxw-2.8-include.diff which only changes the files in the include subdirectory. If you only apply the patch wxwidgets-tiny.diff, I would think this patch would also be needed to ensure that programs which include wxwidgets headers don't suck in the _T() macro. No?

[BrianCameron]

Note the only difference between the wxwidgets-tiny.diff and the patch in comment #18 is that the #ifdef's were not correct, and the patches needed to be a bit reworked to apply against latest 2.8 source.

[BrianCameron]

I spoke too soon. The wxwidgets-tiny.diff does allow you to build the wxWidgets codebase with no problems. However, when I try to build code that uses wxWidgets with the Sun Studio compiler it fails. For example, when I try to build audacity, I get this error when building the audacity 1.3.7 src/Track.cpp file

"/opt/SUNWspro/prod/include/CC/Cstd/./map", line 229: Error: L is not defined.
"Track.cpp", line 753: Where: While instantiating "std::map<BlockFile*, wxLongLongNative, std::less<BlockFile*>, std::allocator<std::pair<BlockFile*const, wxLongLongNative>>>::operator[](BlockFile*const&)".
"Track.cpp", line 753: Where: Instantiated from non-template code.

The error about the "L is not defined" is obviously coming from the way the _T() macro is defined:

#define wxT(x) wxCONCAT_HELPER(L, x)

Based on the above error, I was able to recreate the same problem with a small standalone program, which generates the same error.

#include <float.h>
#include <wx/file.h>
#include <wx/textfile.h>
#include <wx/log.h>

#include "BlockFile.h"
#include <map>

void GetSpaceUsage()
{

std::map<BlockFile*,wxLongLong> blockFiles;

blockFiles[0] = NULL;

std::map<BlockFile*,wxLongLong>::const_iterator bfIter;

}

So, it seems that simply defining the _T macro causes problems with the Sun Studio compiler.

Based on this research, I think if we could use the  http://yippi.hypermall.com/wxw-2.8-smaller.diff patch, it would work better and completely avoids the problem. To me the wxwidgets-tiny.diff seems hackish, and it doesn't surprise me that a gotcha like this appears when using it.

Or, I suppose another approach would be to use the wxwidgets-tiny.diff and somehow rejigger the header files so that the _T() macro only gets defined on Solaris when building the wxWidgets code, but doesn't get defined when building code that uses the wxWidgets header files.

Thoughts?

[vadz]

Sorry, I don't understand why does this error appear exactly. But to answer your last question: WXBUILDING is defined when building wx itself so it would be enough to test for it.

[BrianCameron]

The reason the error

So, do you want me to rework the patch so that wxwidgets-tiny.diff doesn't get defined on Solaris at all when WXBUILDING is not defined, and go with that patch? Or do you want to go with the  http://yippi.hypermall.com/wxw-2.8-smaller.diff patch?

[vadz]

Something seems to have eaten part of your reply.

I'd prefer to make the minimal amount of changes to 2.8 so not defining _T when not building wx would be preferable, yes.

Thanks!

[BrianCameron]

Using "#ifdef WXBUILDING" in the header file does not seem to work. In fact, I can't find WXBUILDING (or any macro like it) defined anywhere in the 2.8.10 version of wxWidgets. Are you sure this is the right macro?

[vadz]

Oops, sorry, WXBUILDING is only defined in svn trunk (see build/bakefiles/common.bkl). I guess it should be pretty safe to backport it to 2.8, at least I don't see what could be broken by it...

[BrianCameron]

patch that allows building of wxwidgets

[BrianCameron]

patch fixing other common-only include files

[BrianCameron]

Okay, finally found some time to work on this. The problem with the wxwidgets-tiny.diff patch is that it redefines the _T() macro in the afterstd.h file and this causes it to get picked up by files which include certain headers.

I just added two patches which fix the problem.

wxwidgets-02-Tmacro.diff fixes wxWidgets so it can build itself without causing problems with the _T macro. However with only this patch you can't build other things since _T() isn't defined. This patch adds the WXBUILDING macro into the 2.8 branch. I think its correct, but you should review.

wxwidgets-03-include.diff fixes only the common header files (not the OS specific ones like macos, msw, palmos, etc.) to not use the _T() macro and instead use the wxT() macro. It also modifies the utils/wxrc/wxrc.cpp file since it seemed that wasn't working with the WXBUILDING macro and it just seemed easier to fix this one file rather than introduce WXBUILDING here.

I have verified that this works, and I can build some other wxWidgets based programs like audacity using wxwidgets 0.2.8 built with these two patches.

I am hoping this can go upstream into the 2.8 branch.

I don't think I can make it any smaller.

[BrianCameron]

I provided the last patch 4 weeks ago, and no comment. I really have worked hard to try and get this working and upstream so that we can better support wxWidgets based programs on Solaris. Unless we are able to fix this in the 2.8 branch, I don't think end-users will be able to build any wxWidgets-based programs until 2.10 is released, which would be too bad.

[vadz]

Sorry for the delay, I didn't have time to look at it but as 2.8 branch doesn't change so much the patches still apply and nothing is lost.

I do have a question however: with this approach, why do we need to test for WXBUILDING at all? I.e. why not always undefine _T in wx/beforestd.h? There is no harm in doing it like it's done now but I just don't understand why is it needed.

Thanks!

[vadz]

Sorry, one more question: isn't the test #ifndef _T in wx/afterstd.h wrong? As it will always fail (because standard headers do define _T) _T will never be defined when using Sun CC. Is this really what we want? IOW are there still any problems if we redefine _T after standard headers inclusion?

[BrianCameron]

Your questions are regarding the wxwidgets-02-Tmacro.diff patch. This patch was designed to fix the issues when compiling wxWidgets with Sun Studio and also to have minimal impact when using other compilers. To make it possible to compile wxwidgets on Solaris without needing to change all the existing usage of _T() to wxT() in the actual code (meaning the .cpp files), it is necessary to make sure that "_T()" is only defined when compiling wxwidgets itself, but not when the STL headers are included. This patch does do this.

You are right that we probably do not need to test for WXBUILDING in wx/beforestd.h. We could just always
undefine it when using the Sun Studio compiler, as you suggest. However, we do only want to ever define _T() in wx/wxchar.h or reset it in wx/afterstd.h if actually building the wxWidgets code, so we do need to use
WXBUILDING in wx/afterstd.h and wx/wxchar.h). This is because, as comment #26 highlights, this hack only
works for building wxWidgets itself, and does not work when building other programs which use wxWidgets and
which also use certain STL features.

To me, it seems better to be consistent. Since we need to only define _T() in wx/wxchar.h and reset it in
wx/wxafterstd.h when WXBUILDING is defined and when using the Sun Studio compiler, it seems better to be
consistent and use the same checks in wx/beforestd.h. However, if you really think modifying the patch to
always undefine it when using the Sun Studio compiler and not check for WXBUILDING in wx/beforestd.h, then I
could make that change and provide an updated patch.

The #ifndef _T in wx/afterstd.h is not wrong. In fact, if I remove the changes to wx/afterstd.h from the patch, and try to compile wxWidgets, then I get these errors when building:

pkgbuild: "./src/common/appbase.cpp", line 342: Error: The function "_T" must have a prototype.
pkgbuild: "./src/common/appbase.cpp", line 343: Error: The function "_T" must have a prototype.
pkgbuild: "./src/common/appbase.cpp", line 353: Error: The function "_T" must have a prototype.
pkgbuild: "./src/common/appbase.cpp", line 377: Error: The function "_T" must have a prototype.
pkgbuild: "./src/common/appbase.cpp", line 424: Error: The function "_T" must have a prototype.

The reason we need to redefine _T() in wx/afterstd.h is because we earlier undefined it in wx/beforestd.h. So, we need to redefine it if it was undefined in wx/beforestd.h, or else we get errors like the above. The "_T()" macro is referred to in places after the afterstd.h header file is included, so it needs to be redefined again.

Initially there was some hope that just undefining _T() in beforestd.h and redefining it in afterstd.h would be all that is needed to address this issue. However, as mentioned in comment #26, this approach caused some programs which use wxWidgets to fail to build, such as audacity. So, it then became clear that to fix this problem, two changes are needed:

- The wxwidgets-02-Tmacro.diff fixes wxWidgets so that the wxWidgets code itself can be compiled. It does this

by ensuring that the _T() macro is only defined when building wxWidgets itself via the WXBUILDING macro, and
not defined when the STL headers are accessed.

- The wxwidgets-03-include.diff is a straightforward patch that just simply changes the commonly used header

files to use wxT() instead of _T(). This way we can always make sure that _T() is never defined when
building other programs that use wxWidgets, avoiding the sorts of issues in comment #26.

Is this more clear, or do you have further questions?

[vadz]

Replying to BrianCameron:

Your questions are regarding the wxwidgets-02-Tmacro.diff patch.

Yes, sorry for not making it clear.

You are right that we probably do not need to test for WXBUILDING in wx/beforestd.h. We could just always
undefine it when using the Sun Studio compiler, as you suggest. However, we do only want to ever define _T() in wx/wxchar.h or reset it in wx/afterstd.h if actually building the wxWidgets code, so we do need to use
WXBUILDING in wx/afterstd.h and wx/wxchar.h). This is because, as comment #26 highlights, this hack only
works for building wxWidgets itself, and does not work when building other programs which use wxWidgets and
which also use certain STL features.

I see. Sorry for missing/forgetting comment:26.

To me, it seems better to be consistent. Since we need to only define _T() in wx/wxchar.h and reset it in
wx/wxafterstd.h when WXBUILDING is defined and when using the Sun Studio compiler, it seems better to be
consistent and use the same checks in wx/beforestd.h. However, if you really think modifying the patch to
always undefine it when using the Sun Studio compiler and not check for WXBUILDING in wx/beforestd.h, then I
could make that change and provide an updated patch.

I don't think I need an updated patch for this, just doing this should be fine, shouldn't it?

include/wx/beforestd.h

@@ -63 +63 @@
     #pragma warning(disable:4786)
 #endif // VC++ < 7
 
+/*
+    Recent versions of Sun C++ compiler use _T in their standard headers and
+    our definition of it in wx/wxchar.h conflicts with them and breaks
+    compilation, so undefine _T before including them and redefine it back in
+    wx/afterstd.h.
+ */
+#if defined(__SUNPRO_CC) || defined(__SUNPRO_C)
+    #undef _T
+#endif /* SUNCC */
+

The #ifndef _T in wx/afterstd.h is not wrong. In fact, if I remove the changes to wx/afterstd.h from the patch

Sorry for being unclear again. I do realize we need to define _T again here. What I meant to ask was why did we need to have this #ifndef. I.e. why can't we just remove it or replace it with _T. Removing it should work now as it was undefined before and apparently isn't defined as a macro in the standard headers. But this still leaves the door open to _T being defined as a macro in standard headers later so maybe undefining it is even better. Have I managed to explain myself better this time?

Is this more clear, or do you have further questions?

Just one: would wxrc.cpp compile without all these changes if _T were defined? If so, could we add a check for some wxNEEDS__T in wx/wxchar.h and define _T even for Sun compiler if it's defined? This would avoid the need for the changes in wxrc (other than adding #define wxNEEDS__T to the top of it) and could be helpful to other people compiling their code using _T with Sun CC.

Thanks again!

[BrianCameron]

Yes, your proposed change to beforestd.h to only undefine _T if "SUNPRO_CC SUNPRO_C" is defined would be fine.

I assume you are talking about this line in afterstd.h:

+#ifndef _T
+#if !wxUSE_UNICODE
+#define _T(x) x
+#else /* Unicode */
+/* use wxCONCAT_HELPER so that x could be expanded if it's a macro */
+#define _T(x) wxCONCAT_HELPER(L, x)
+#endif /* ASCII/Unicode */
+#endif /* !defined(_T)

Yes, it would be fine to remove the "#ifndef _T" part of this check, or replace it with code that undefines _T() and then just resets it.

would wxrc.cpp compile without all these changes if _T were defined?

Yes. There really is no issue with the _T() macro in this file. I suppose I also could have fixed this problem by figuring out how to hack this code so that WXBUILDING is defined only when building, so it works like the rest of wxWidgets. But it just seemed easier to change the _T() references to wxT() in this one file.

If so, could we add a check for some wxNEEDST in wx/wxchar.h and define _T even for Sun compiler if it's
defined? This would avoid the need for the changes in wxrc (other than adding #define wxNEEDST to the top
of it) and could be helpful to other people compiling their code using _T with Sun CC.

Yes, this would be a fine way to also solve the problem with this file. Also, as you say, being able to define wxNEEDST to get the _T() macro may be useful for some other people compiling their code using _T if they want, assuming that their code doesn't make use of the sorts of features that cause the code to not compile when _T() is defined.

You say you don't need an updated patch. Just to be sure, would you be able to make these changes after you apply the patches, or would you like me to provide updated patches?

I really appreciate all your help with this. I am just really interested and excited to get wxWidgets better supported on Solaris. The team at Sun who is responsible for wxWidgets really only cares about a single application that they deliver (which doesn't exhibit the problems like audacity does), and seem disinterested in patching the code to make it work better unless the patch gets accepted upstream. I would like to eventually integrate audacity into Solaris, so the team I work for is more interested to see this problem get fixed.

[vadz]

Work around conflict between _T() and Sun CC standard headers.

[vadz]

Define WXBUILDING when building wxWidgets itself.

[vadz]

Don't define _T() when using Sun compiler if possible.

[vadz]

Don't use _T() in public headers used under Unix.

[vadz]

Predefine wxNEEDS_T to fix wxrc compilation with Sun CC.

[vadz]

Replying to BrianCameron:

You say you don't need an updated patch. Just to be sure, would you be able to make these changes after you apply the patches, or would you like me to provide updated patches?

I've attached the patches which I have in my local branch so far. Please let me know if they work for you -- if they do I'll commit them to 2.8 branch. If not, please send me patches against my local version (i.e. against the tree obtained by applying the patches above, in order from 0001 to 0005, to the latest 2.8 svn). TIA!

I really appreciate all your help with this. I am just really interested and excited to get wxWidgets better supported on Solaris.

I'm interested in this too if only because I have a soft spot for Solaris as this was the first Unix system I worked with (well, it was still SunOS back in 1991...). But I need to take care to avoid breaking things in stable branch and this means painstakingly reviewing the patches which takes time.

The team at Sun who is responsible for wxWidgets really only cares about a single application that they deliver

What is it if it's not a secret?

(which doesn't exhibit the problems like audacity does), and seem disinterested in patching the code to make it work better unless the patch gets accepted upstream. I would like to eventually integrate audacity into Solaris, so the team I work for is more interested to see this problem get fixed.

Good luck with this and thanks again for your help with this issue!

[BrianCameron]

update 0001 patch that works

[BrianCameron]

I understand that we don't want to break wxWidgets stable branch. I think we have worked hard to make sure that these changes should be harmless, and should not break things for anyone. Aside from the headaches Solaris users will have with the fact that it is a bit of a problem to use the wxWidgets _T() macro, anyway. But I don't think that is avoidable.

Thanks for the patches. I just tested building wxWidgets itself with the patches you provided, and then building audacity. However, building audacity failed as described in comment #26 because the patch is re-assigning the _T() macro in afterstd.h.

I provided an updated patch, which just adds two lines and surrounds the re-assign of the _T() macro with the following #ifdef:

+#if defined(WXBUILDING) defined(wxNEEDS_T)
[...]
#endif /* defined(WXBUILDING) */

This way, the _T() macro only gets reset if WXBUILDING is set, or if wxNEEDS_T is set. Note that I found that the wxrc.cpp file would not compile if the above line did not check for wxNEEDS_T, so that's why I added that as well.

So, I think this is ready to go upstream if you think the above minor change is acceptable.

Oh, by the way, the program that Sun ships that uses wxWidgets is a program called pgadmin, a PostgreSQL admin tool. It is no secret, I think.

[BrianCameron]

Also, note, just in case it isn't clear. I was able to build wxWidgets and audacity with the patches after making the minor change in the updated patch.

[VZ]

(In [61869]) Define WXBUILDING when building wxWidgets itself.

This is needed by the upcoming _T-related patches (see #10660).

Notice that only Makefile.in was regenerated using the old 0.2.5 bakefile
version to keep changes to the minimum. The other makefiles will have more
changes when they are regenerated with bakefile 0.2.6 after we update to it.

[VZ]

(In [61870]) Don't define _T() when using Sun compiler if possible.

Avoid defining _T() if possible as it conflicts with the use of this
identifier in standard headers. Do still define it when building wx itself or
when the special symbol wxNEEDST is explicitly predefined.

See #10660.

[VZ]

(In [61871]) Undef _T before including standard headers and redefine it later.

This change fixes the build of wxWidgets itself by undefining _T() before
including any standard headers and redefining it after including them.

See #10660.

[VZ]

(In [61872]) Don't use _T() in public headers used under Unix.

Avoid conflict with the Sun CC standard headers (see #10660).

Also update the change log to mention _T() changes.

[VZ]

(In [61873]) Predefine wxNEEDS_T to fix wxrc compilation with Sun CC.

After the recent changes _T() is not defined any longer when using Sun CC but
this file does need it to be defined and seems to compile fine when it is, so
define wxNEEDS_T before including any wx headers.

See #10660.

[vadz]

Thanks, applied all patches now (with your correction, don't know how could I forget this...), please let me know if anything is still wrong.

Thanks again!

[BrianCameron]

You are awesome! Any idea when the next release of wxWidgets 2.8 will be released with this fix? I can't wait!

[vadz]

This bug would probably never have been fixed in 2.8 without your help so any thanks should go to you.

As for 2.8.11, I think it could be the last 2.8 release so we'll probably want to apply more fixes (see  list of bugs to backport to it before making the release. Hopefully it should happen before end of the year but I prefer to give no promises...