> On Apr 4, 2019, at 9:36 AM, Erik Joelsson <erik.joels...@oracle.com> wrote: > > Hello Kim, > > On 2019-04-03 16:34, Kim Barrett wrote: >>> On Apr 3, 2019, at 9:51 AM, Erik Joelsson <erik.joels...@oracle.com> >>> wrote:Additionally, I would like to replace all (or at least most) >>> instances of __FILE__ with my new THIS_FILE, and I suspect some other >>> places would be more performance sensitive. Do you see any problem with >>> doing that substitution? >> Given that our build system is mostly unhappy with basename collisions >> (other than open/custom overrides), I don't think there's much of a >> usage problem with using a basename-like approach everywhere either. > > It's also allowed, though perhaps not used, to override less specific files > with more specific, as in OS specific file foo.cpp would override shared > foo.cpp.
I didn’t know that. I don’t think that feature is being used, though could be mistaken. > I won't update the other references in this patch. I will see if anyone else > thinks it's a good enough idea to give it a go. That kind of change would not > affect the build, nor the full paths in binaries, just (hopefully) improve > readability of error/log messages. OK. >> While researching what various platforms do in this area, I ran across >> a suggestion that debug builds should maybe use full paths and release >> builds use shortened paths. That seems like an interesting idea, though >> we still want short paths in the exceptions.hpp case. > If you really think this is worth it, I could implement it, but I would > rather keep it the same if possible. Not sure. The point would be to provide full paths in builds that developers are using, but not expose full paths in shipped binaries. But I don’t have a strong opinion. >>>> This doesn't eliminate the full pathname string embedded in the >>>> executable, but I don't think the proposed FILE_MACRO_OFFSET will do >>>> that either. Those get fixed by -fmacro-prefix-map=. >>> Correct. While solving this universally would be nice, it wasn't the goal >>> of this bug. The main goal was to get precompiled headers to work with GCC >>> again. >> I agree that either approach does that. I looked at the webrev and >> it's a bunch of build system changes that I don't feel qualified to >> review, so I suggested an alternative that I understand. We each have >> our preferred tools. I don't object to the build-system-based >> approach, just can't give an actual thumbs-up. > > Right, and I appreciate the input! But could you at least review the hotspot > part, so I can get someone else to review the build part? OK, I can do that. ------------------------------------------------------------------------------ src/hotspot/share/utilities/macros.hpp 645 #if FILE_MACRO_OFFSET 646 #define THIS_FILE (__FILE__ + FILE_MACRO_OFFSET) 647 #else 648 #define THIS_FILE __FILE__ 649 #endif Is the "#if FILE_MACRO_OFFSET" an intentional test for 0, or is this an implicit test for "defined"? If the former, e.g. we're assuming it will always be defined but might have a 0 value, then I'd skip it and just unconditionally define THIS_FILE as (__FILE__ + FILE_MACRO_OFFSET). If the latter, some compilers will (with some warning levels or options, such as gcc -Wundef) complain about the (specified by the standard) implicit conversion to 0 for an unrecognized identifier in an #if expression, and an #ifdef should be used to protect against that. ------------------------------------------------------------------------------
