To comment on the following update, log in, then open the issue:
http://www.openoffice.org/issues/show_bug.cgi?id=63927
------- Additional comments from [EMAIL PROTECTED] Wed Apr 12 07:51:50 -0700
2006 -------
Response to review comments:
> - shlib.cxx, first "#ifdef LINUX" block: It should be stated in a
> comment that the complete block is only a temporary hack (otherwise,
> hacks like the extern get_this_libpath thing should not be done in
> such a dirty way).
done.
> - shlib.cxx, "These are loaded at startup": What is the criterium
> for including a shared library from program/ in this list? Some
> that are loaded at startup are not included (e.g., libuno_sal.so.3),
> some that are not UNO components are included (e.g., libreg.so.3),
> some that export symbols other than _ZT[IN]* or component_* are
> included (e.g., behelper.uno.so).
libreg is the only non-component - I removed it. wrt. exporting symbols -
that's fine, if you need them you have to link at compile time => no issue.
> - shlib.cxx: "!rLibName.match(pLookup[i])" evaluates to true when
> the two do not match---is that intended?
urg yes - too used to strcasecmp; fixed.
> - shlib.cxx: There is no get_this_libpath anywhere in sal
> SRC680m160.
no ? not in bootstrap.cxx ? - cvs history says it is.
> - shlib.cxx, "...symbols in a signal, global...": Should read
> "...symbols in a single, global..."
fixed.
> - shlib.cxx, lcl_isInternalLibrary: Use OSL_TRACE or similar instead
> of fprintf.
commented out, but still fprintf ;-) never trusted/liked OSL_TRACE.
> - shlib.cxx, lcl_isInternalLibrary: The name of the exlink library
> should probably be "libexlink.so" instead of
> "libexlink680li.so"---merely depending on the offapi UNOIDL types is
> generally not considered depending on SUPD.
Hmm, ok - but of course it's C++ compiler ABI dependant, also,
it's necessary to have -all- UNO types in there; but as you like -
easy to fix, I leave it to you ?
> - shlib.cxx, lcl_isInternalLibrary: sal is part of URE, but
> libexlink.so cannot be part of URE, as it contains information about
> OOo-specific (non-URE) UNO types. The hard-coded approach to dlopen
> libexlink.so from within sal when needed is thus too naive.
Yes - of course ;-) if you can come up with a better plan I'm all
for it. We can register a callback with UNO I guess from some upper
level - but ... this is your world. What is needed is clear, how to
structure it is not (to me).
> - shlib.cxx, "// else - faster local only binding": The formatting
> of this comment is confusing---what does it refer to?
the case where it is an internal library;
ie. !SAL_LOADMODULE_GLOBAL is 'local', so ...
> - cppumaker.cxx, "/catch.hxx": I would expect the name of this file
> to be specified on the command line (rationale: its contents depends
> on what types you specify on the command line, so there is not *the*
> universal catch.hxx). (Thinking about it, writing out a single
> catch.hxx might be too naive, in that it forces us to stay with the
> current approach of calling cppumaker only once for all types
> together, instead of more selectively for subsets.)
ok - fine; I added the -E argument.
> - cppumaker.cxx: Why have both aExceptionNames and aIncludes, if the
> members of one can be computed from the members of the other (via
> scopedName)?
Fixed, things changed here for the better from 2.0.2 to m161 I
think, easier now.
> - cppumaker.cxx: Minor issue: "aStr.getStr()" is better than "(const
> sal_Char *) aStr".
Fixed - it's ok to use FILE / stdio stuff here ? /me never builds
on win32 so ...
> - cpputype.cxx: Minor issue: "aExceptionNames.push_back(aTypeName)"
> is more idiomatic than
> "aExceptionNames.insert(aExceptionNames.end(), aTypeName)".
fixed.
> - cpputype.hxx: There is no do_internal anywhere in codemaker
> SRC680m160.
fixed.
> - except.cxx: If I understand correctly, catch.hxx only includes
> .hpps for exception types; does this cause any .hpps to be
> recursively included that have problems with major, minor, auto?
Nah - the major/minor/auto stuff is a throw-back to trying to
share much more code: which turned out to be a waste of time ;-)
> - Please use no tabs, and no more than 80 characters per line. This
> is a OOo policy that often gets broken, and seems not to be
> documented anywhere any longer, but I at least still consider it an
> important policy, so I cannot finish this review without mentioning
> it. ;)
I use (or should use) 4 stop tabs (as instructed), seriously - if
you read the diff it looks terrible: this is because the whole unix
terminal, <insert-rest-of-world> etc. uses 8 stop tabs ;-) of course
when you apply the diff & load in your text editor things should look
fine [ I hope ].
---------------------------------------------------------------------
Please do not reply to this automatically generated notification from
Issue Tracker. Please log onto the website and enter your comments.
http://qa.openoffice.org/issue_handling/project_issues.html#notification
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
