git grep "\(\b\|f\)printf" shows that there about 3100 calls to various printf functions. A substantial part of these are debugging output from C++ source files; those should be replaced with SAL_INFO typically. There is also a substantial part which should *not* be replaced: * printf calls in shell scripts and Makefiles and AWK and Perl * C source files * ancient code in */test and */workben directories * printf that are not for debugging but printing help messages for the end user * printf that are not for debugging but producing output from some tool * printf in external/ and odk/ and onlineupdate/source/libmar/ * printf in sal/ (there are bootstrapping problems here) To a first approximation, a good heuristic is that a printf should be replaced if it's inside a #ifdef OSL_DEBUG_LEVEL or #ifdef DBGUTIL or similar. For an example commit see https://gerrit.libreoffice.org/plugins/gitiles/core/+/a8e83ce1d86892061603159f5c4460803d077fd5%5E!/ The first argument to SAL_INFO is a "log area", see include/sal/log-areas.dox for existing ones; it may be necessary to add new log areas there.
...and see <https://wiki.documentfoundation.org/Development/GeneralProgrammingGuidelines#Assertions_and_Logging> for a general discussion when to use SAL_INFO and SAL_WARN
Here is some query that might help find the right places: https://opengrok.libreoffice.org/search?project=core&full=fprintf&defs=&refs=&path=%21external+%21sal+%21odk+%21onlineupdate+%21workben+%21test+%21idlc+%21helpcompiler+%21i18npool+%21registry+%21svx%2Fsource%2Fgengal%2F+%21smoketest+%21l10ntools+%21cpputools+%21extensions+%21codemaker+%21desktop%2Fsource%2Fapp%2F+%21vcl%2Funx%2Fgeneric%2Fprint%2F+%21desktop%2Fsource%2Fminidump%2F+%21desktop%2Fsource%2Fdeployment%2Fmisc%2F+%21cli_ure%2Fsource%2Fclimaker%2F+%21libreofficekit&hist=&type=cxx&n=25 But please still follow the hints from comment 0, there is no guarantee that all results need changing.
I have been working on this issue for a while now. Since there are many files to modify, I uploaded a WIP on gerrit: https://gerrit.libreoffice.org/c/core/+/90729. There are a couple of things that I am not sure how they are supposed to be implemented. Mainly things like line 336 in "bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx". There are a couple of for-loops for printing some debugging information like: for (...) fprintf(stderr, "..., ", ...); Which I turned into: std::ostringstream oss; for (...) oss << "..." << ... << ", "; SAL_INFO(/* area */, oss); Should I do something else?
(In reply to Yukio Siraichi from comment #3) > There are a couple of things that I am not sure how they are supposed to be > implemented. Mainly things like line 336 in > "bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx". There are a couple of > for-loops for printing some debugging information like: Especially for an easy hack, please only change code that you can build locally. bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx is only built when building for Alpha CPUs on Linux (see bridges/Library_cpp_uno.mk), so it is unlikely that your local build would actually build that code. Better leave such code alone then.
For printf in test code, please note <https://gerrit.libreoffice.org/c/core/+/90729/1#message-fb09c15ab1b2231d2f2df2d8e22fcb748264234f>: "For test code like basic/qa/cppunit/, I think turning output into SAL_INFO or SAL_WARN is not a good idea: Test code should unconditionally report all relevant findings. When a (CppUnit, JUnit, etc.) test succeeds, gbuild will normally hide all of the test's output anyway. But when a test fails (esp. one executing on a tinderbox which you cannot debug directly, and for which all you have is the failed test's output), you want to always see all the relevant findings, regardless of the --enable-sal-log configuration and SAL_LOG env var settings."
I think I am almost done. I have some questions on some of the files, though (see below). - hwpfilter/source/formula.cxx There are lots of 'debug' printf's, which more changes are needed for making it happen. The problem is that it is using a global var to keep track of indentation. One way to work around that is by making that variable a field of the 'Formula' class. What do you think? - hwpfilter/source/grammar.cxx - hwpfilter/source/lexer.cxx Has some (at least they look like) 'yacc' code. Should the 'fprintf' commands be replaced here? - idl/source/prj/database.cxx - include/LibreOfficeKit/LibreOfficeKitInit.h - jvmfwk/plugins/sunmajor/javaenvsetup/javaldx.cxx - jvmfwk/plugins/sunmajor/pluginlib/sunjavaplugin.cxx - vcl/osx/salinst.cxx - vcl/source/uipreviewer/previewer.cxx - vcl/unx/generic/app/i18n_cb.cxx - vcl/unx/generic/app/saldata.cxx The 'fprintf's here look like they are supposed to show up to some end-user (there are many other files that I am omitting here). - oox/source/helper/propertymap.cxx Data is being dumped in this file. It feels odd to replace them with SAL_* functions. Am I doing it right? - sdext/source/pdfimport/xpdfwrapper/pdfioutdev_gpl.cxx These are plain 'printf's. I feel like they should be replaced, but it just feels strange. - sw/source/core/bastyp/calc.cxx It is a 'printf' inside a 'main' function. - unotools/source/i18n/localedatawrapper.cxx It has a 'SAL_WARN' just after the 'fprintf's. Finally, there are lots of 'printf's that use tabs or spaces for indenting debugging information. Should I leave them there? I feel that (at least when the data is not structured) there is no need for the indentation (not the case of 'hwpfilter/source/formula.cxx').
ping
(In reply to Yukio Siraichi from comment #6) > I think I am almost done. I have some questions on some of the files, though > (see below). Could you please submit the patch to gerrit? I think it's a better place to discuss technicalities than bugzilla
(In reply to Yukio Siraichi from comment #6) > I think I am almost done. I have some questions on some of the files, though > (see below). > > - hwpfilter/source/formula.cxx > There are lots of 'debug' printf's, which more changes are needed for making > it happen. The problem is that it is using a global var to keep track of > indentation. One way to work around that is by making that variable a field > of the 'Formula' class. What do you think? hmm that's a bit odd, probably just replace the printf for a start and keep the macros around it? > - hwpfilter/source/grammar.cxx > - hwpfilter/source/lexer.cxx > Has some (at least they look like) 'yacc' code. Should the 'fprintf' > commands be replaced here? probably this is generated code, better not replace there. > - idl/source/prj/database.cxx > - include/LibreOfficeKit/LibreOfficeKitInit.h > - jvmfwk/plugins/sunmajor/javaenvsetup/javaldx.cxx > - jvmfwk/plugins/sunmajor/pluginlib/sunjavaplugin.cxx > - vcl/osx/salinst.cxx > - vcl/source/uipreviewer/previewer.cxx > - vcl/unx/generic/app/i18n_cb.cxx > - vcl/unx/generic/app/saldata.cxx > The 'fprintf's here look like they are supposed to show up to some end-user > (there are many other files that I am omitting here). agree, don't replace. > - oox/source/helper/propertymap.cxx > Data is being dumped in this file. It feels odd to replace them with SAL_* > functions. Am I doing it right? so there's definitely some code-generating printfs in the 2nd half of this file that shouldn't be replaced; it's not obvious to me what the #ifdef'd printfs in the 1st half are doing. > - sdext/source/pdfimport/xpdfwrapper/pdfioutdev_gpl.cxx > These are plain 'printf's. I feel like they should be replaced, but it just > feels strange. this is an exectuable that writes to stdout, don't replace. > - sw/source/core/bastyp/calc.cxx > It is a 'printf' inside a 'main' function. that's some weird code, best don't touch. > - unotools/source/i18n/localedatawrapper.cxx > It has a 'SAL_WARN' just after the 'fprintf's. that's weird, perhaps the author of that code could tell you why that is, that's Eike Rathke most likely. > Finally, there are lots of 'printf's that use tabs or spaces for indenting > debugging information. Should I leave them there? I feel that (at least when > the data is not structured) there is no need for the indentation (not the > case of 'hwpfilter/source/formula.cxx'). it might be helpful in some cases; the SAL_WARN are prefixed with a generated string but the indentation could just go behind the prefix. PS: i see there's a patch in gerrit and it changes 1000 lines - that's a bit large to review, would it be possible to split that up into smaller chunks?
Yukio Siraichi committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/317c05a88f79096422681a23b61aa6ae9f3f084b tdf#130924 [vcl] replace `*printf`s with `SAL_*` logging macros. It will be available in 7.0.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
(In reply to Michael Stahl (CIB) from comment #9) > > - unotools/source/i18n/localedatawrapper.cxx > > It has a 'SAL_WARN' just after the 'fprintf's. > > that's weird, perhaps the author of that code could tell you why that is, > that's Eike Rathke most likely. That code underwent some iterations.. the original was fprintf( stderr, "\n%s\n", pStr); fflush( stderr); DBG_ERROR( pStr); that outputs the message on console (the function is called only in a debug build or if some environment variable is set and was meant to aid locale data developers to check their addition in a non-debug environment) and then wrote it to the debug utility that since long is gone, also because on Windows one never saw console output from a GUI program, don't know if that ever changed. Next iteration (not by me) was - DBG_ERROR( pStr); + OSL_TRACE("%s", pStr); probably just to get rid of DBG_ERROR(). Then followed - OSL_TRACE("%s", pStr); + SAL_WARN("unotools", pStr); So that looks duplicated, but for the non-debug program run purpose if then the SAL_WARN() should be removed, not the fprintf()/fflush(). Iff Windows does actually display console output. Or so..
Dear Yukio Siraichi, This bug has been in ASSIGNED status for more than 3 months without any activity. Resetting it to NEW. Please assign it back to yourself if you're still working on this.
You should try to choose between SAL_INFO and SAL_WARN based on the printed content, and the context. If you are about to print some INFORMATION, then choose SAL_INFO, and if it a WARNING then you should use SAL_WARN. This is useful for deciding: https://bugs.documentfoundation.org/show_bug.cgi?id=91794#c0 If not understandable from the context, you can choose the equivalent replacement: ----- #if OSL_DEBUG_LEVEL > 2 fprintf(...) #endif ----- should become: SAL_INFO(...) ----- #if OSL_DEBUG_LEVEL > 1 fprintf(...) #endif ----- should become: SAL_WARN(...) ----- #if OSL_DEBUG_LEVEL > 0 fprintf(...) #endif ----- should become: SAL_WARN(...) Please note that you should remove '#if OSL_DEBUG_LEVEL > x' and '#endif'.
Emircan Agac committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/617fc3a0ed28b0ccc7fa658e4ba13a0fc9fb353b tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Emircan Agac committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/42f536df5c5b15d34c7c5663c6653d5bedbc2dc2 tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Emircan Agac committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/7a8ec6cddc9f37ba6ff1a98ad39702521c8fb36b tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
pragat-pandya committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/6f121860d0537060084278da11842732a748d6b7 tdf#130924 replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
offtkp committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c9b69e412b9646f7d49f500d47a078c2ea155c8c tdf#130924 replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
ektagoel12 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/307b1cc494c51c802882ae23029e33a72d57f2a3 tdf#130924 replace debugging fprintf calls with SAL_INFO/SAL_WARN It will be available in 7.6.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Yousef_Rabia committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/a76ef6e4cb345d6aa70f48777708188fce0ceacb tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN It will be available in 7.6.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
sahil committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/df0db4132428e95d276e923d8d5fb603dbb7f7bb tdf#130924 replace '*printf' with 'SAL_*' logging macros. It will be available in 24.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Ilmari Lauhakangas committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/115c8b42b8db7882f3e10e7ff1ef2f48d3ef973c Revert "tdf#130924 replace '*printf' with 'SAL_*' logging macros." It will be available in 24.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Re-evaluating the EasyHack in 2023 This issue is still relevant, as there are many instances remaining. Please note that as described in comment 1, not all of these should be touched. $ git grep -w printf *.cxx | wc -l 687 $ git grep -w fprintf *.cxx | wc -l 1442
sahil committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/d56b61975330329c5dc1e99cc7750a690d3206ed tdf#130924 replace '*printf' with 'SAL_*' logging macros in tools It will be available in 24.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
sahil committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c0e687edf47f9bd2735a4e71116a9c1e50900de6 tdf#130924 replace '*printf' with 'SAL_*' logging macros in pyuno It will be available in 24.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Yli875 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/2ee71dff5cdc7ddd59f249e97d3c4154fa28d8fd tdf#130924 replace debugging printf with SAL_INFO It will be available in 24.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.