To comment on the following update, log in, then open the issue:
http://www.openoffice.org/issues/show_bug.cgi?id=63927
User sb changed the following:
What |Old value |New value
================================================================================
CC|'kr' |'jsc,kr'
--------------------------------------------------------------------------------
Assigned to|sb |mmeeks
--------------------------------------------------------------------------------
------- Additional comments from [EMAIL PROTECTED] Tue Apr 4 04:56:46 -0700
2006 -------
@mmeeks: Just to clarify, do you want me to handle this patch, or do you only
want me to review it and want to put it into a CWS yourself?
CC'ing jsc in case he has additional input regarding cppumaker.
The list of issues I see with the patch:
- 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).
- 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).
- shlib.cxx: "!rLibName.match(pLookup[i])" evaluates to true when the two do
not match---is that intended?
- shlib.cxx: There is no get_this_libpath anywhere in sal SRC680m160.
- shlib.cxx, "...symbols in a signal, global...": Should read "...symbols in a
single, global..."
- shlib.cxx, lcl_isInternalLibrary: Use OSL_TRACE or similar instead of
fprintf.
- 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.
- 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.
- shlib.cxx, "// else - faster local only binding": The formatting of this
comment is confusing---what does it refer to?
- 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.)
- cppumaker.cxx: Why have both aExceptionNames and aIncludes, if the members of
one can be computed from the members of the other (via scopedName)?
- cppumaker.cxx: Minor issue: "aStr.getStr()" is better than "(const sal_Char
*) aStr".
- cpputype.cxx: Minor issue: "aExceptionNames.push_back(aTypeName)" is more
idiomatic than "aExceptionNames.insert(aExceptionNames.end(), aTypeName)".
- cpputype.hxx: There is no do_internal anywhere in codemaker SRC680m160.
- 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?
- 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. ;)
---------------------------------------------------------------------
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]
