The uconv code consists of a number of small C++ compilation units which are easy to convert to a unified build. On my machine, this speeds up the compilation of this directory by 4 seconds on 8 cores. https://tbpl.mozilla.org/?tree=Try&rev=5da048668239

Comment on attachment 818836 [details] [diff] [review] Build the uconv library in unified mode Review of attachment 818836 [details] [diff] [review]: ----------------------------------------------------------------- (nsUnicodeToISO2022CN.* were two empty files which I removed in this patch, but git's diff is confused about them. I'll create a proper patch before landing.)

Comment on attachment 818836 [details] [diff] [review] Build the uconv library in unified mode Review of attachment 818836 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/uconv/util/moz.build @@ +6,5 @@ > > MODULE = 'uconv' > > CPP_SOURCES += [ > + 'UnifiedUCVUtils.cpp', I think it would be better to create those automatically from CPP_SOURCES. That avoids having to edit the unified file when adding or removing cpp files, instead of editing CPP_SOURCES like everywhere else. In fact, I already have a patch with rules to do that.

(In reply to comment #3) > Comment on attachment 818836 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=818836 > Build the uconv library in unified mode > > Review of attachment 818836 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: intl/uconv/util/moz.build > @@ +6,5 @@ > > > > MODULE = 'uconv' > > > > CPP_SOURCES += [ > > + 'UnifiedUCVUtils.cpp', > > I think it would be better to create those automatically from CPP_SOURCES. That > avoids having to edit the unified file when adding or removing cpp files, > instead of editing CPP_SOURCES like everywhere else. That's a bad idea since you may not want to build some of your files in Unified mode if they conflict with each other in unfixable ways. This is not and will never be an automated transition and it is best to leave the decision on which files are built in Unified mode and how to the developers.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) > That's a bad idea since you may not want to build some of your files in > Unified mode if they conflict with each other in unfixable ways. This is > not and will never be an automated transition and it is best to leave the > decision on which files are built in Unified mode and how to the developers. Which is why i want to add a UNIFIED_CPP_SOURCES variable.

(In reply to Mike Hommey [:glandium] from comment #5) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4) > > That's a bad idea since you may not want to build some of your files in > > Unified mode if they conflict with each other in unfixable ways. This is > > not and will never be an automated transition and it is best to leave the > > decision on which files are built in Unified mode and how to the developers. > > Which is why i want to add a UNIFIED_CPP_SOURCES variable. OK, but that doesn't need to block this bug. :-)

Comment on attachment 818836 [details] [diff] [review] Build the uconv library in unified mode Review of attachment 818836 [details] [diff] [review]: ----------------------------------------------------------------- If you're already doing this, would it make sense to put all the converters in a single directory? The current directory structure dates back to when there were separate libraries for each group of encodings, and there's no real need to preserve it as far as I can see. And if you already do /that/, maybe it would be more efficient to regroup the sources to have encoders in one group and decoders in another (and if you want further subdivision you could separate out the multitable en- and de- coders and the algorithmic ones from the simple table ones?

(In reply to Simon Montagu :smontagu from comment #7) > Comment on attachment 818836 [details] [diff] [review] > Build the uconv library in unified mode > > Review of attachment 818836 [details] [diff] [review]: > ----------------------------------------------------------------- > > If you're already doing this, would it make sense to put all the converters > in a single directory? The current directory structure dates back to when > there were separate libraries for each group of encodings, and there's no > real need to preserve it as far as I can see. Hmm, I guess so but I would prefer to do that in a separate bug. > And if you already do /that/, maybe it would be more efficient to regroup > the sources to have encoders in one group and decoders in another (and if > you want further subdivision you could separate out the multitable en- and > de- coders and the algorithmic ones from the simple table ones? My ultimate goal is to unify all of this code in one single cpp file, but I'd like to make baby steps towards that goal, since there is a bit of risk here, and I would like to create better bisection opportunity by moving slowly on this.

Comment on attachment 818836 [details] [diff] [review] Build the uconv library in unified mode Review of attachment 818836 [details] [diff] [review]: ----------------------------------------------------------------- The way I see it, if you want to structure your C++ files this way without the cooperation of the build system, you are free to do that and don't need build module involvement. (As long as you don't break incremental builds, etc.) That being said, checking in unified sources can lead to some gotchas. Not having the original .cpp files annotated in moz.build will also lead to suboptimal IDE project files (when we eventually produce those). Long term, I imagine you'll want to do this properly, with build system integration. But, don't let the build system block on progress. I'm removing myself from review as this is a uconv module decision (for now). That being said, I would encourage you to wait for the generic unified build mode support that glandium referenced.

(In reply to Gregory Szorc [:gps] from comment #9) > The way I see it, if you want to structure your C++ files this way without > the cooperation of the build system, you are free to do that and don't need > build module involvement. (As long as you don't break incremental builds, > etc.) Yeah, I was mostly using the review flag as a way to communicate this change to you. :-) > That being said, checking in unified sources can lead to some gotchas. Like what? > Not > having the original .cpp files annotated in moz.build will also lead to > suboptimal IDE project files (when we eventually produce those). All IDEs that I know of are capable of handling #include's. That's needed for pretty much anything interesting that you may want to do in the IDE. > Long term, I imagine you'll want to do this properly, with build system > integration. But, don't let the build system block on progress. Sure! > I'm removing myself from review as this is a uconv module decision (for > now). That being said, I would encourage you to wait for the generic unified > build mode support that glandium referenced. It's not quite clear to me when that will land. If that's in a few days, I can wait, but I'm not inclined to do that if it's going to take more time, since essentially that will make me rewrite half of this patch. I guess whoever lands second will get to deal with that though. :-)

Comment on attachment 818836 [details] [diff] [review] Build the uconv library in unified mode Review of attachment 818836 [details] [diff] [review]: ----------------------------------------------------------------- I'll buy baby steps :)

Pushed a follow-up to rename the SIZE_OF_TABLES macro.

https://hg.mozilla.org/mozilla-central/rev/833a5441ef8b https://hg.mozilla.org/mozilla-central/rev/8fcdb3653f57