search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

[root6] Rename s/ostrstream/St_ostrstream/ #134

Closed plexoos closed 2 years ago

plexoos commented 2 years ago

Fixes #126 Error due to ambiguous ostrstream while running bfc.C in ROOT6

dmarkh commented 2 years ago

Personally, I never understood a need for St* streams. Victor explained that they needed during times when STL was unstable and they encountered issues with streams - thus wrapper was created. In my code (StDbLib), I never expect St streams, but rather standard std:: streams. Moreover, StDbLib cannot depend on ROOT, as it has a standalone compile option - used by Jeff in the online domain. If St_ streams are ROOT-dependent, please do not use them as a substitute for std:: streams in the StDbLib codes.

perevbnlgov commented 2 years ago

I do not remember all the history, but as a result we have class class ostrstream : public std::ostringstream { But there is an official std::ostrstream. In very old code there were methods :

const char *str() int pcount() const void freeze(bool)

which are not in ostrstream. So this fake ostrstream was used. But when Root6 + new compiler is used this compiler found that there are two ostrstring classes, one in Star code and one in std:: and clashes arised.

Now I replaced my class ostrstring to St_ostrstring and added #define ostrstream St_ostrstream No clashes any more

On 2021-09-02 14:42, Dmitry Arkhipkin wrote:

Personally, I never understood a need for St* streams. Victor explained that they needed during times when STL was unstable and they encountered issues with streams - thus wrapper was created. In my code (StDbLib), I never expect St streams, but rather standard std:: streams. Moreover, StDbLib cannot depend on ROOT, as it has a standalone compile option - used by Jeff in the online domain. If St_ streams are ROOT-dependent, please do not use them as a substitute for std:: streams in the StDbLib codes.

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-911957891 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7PKY44OKZXT3R2ESODT77ASLANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!RziG-qz2QLFAQZwBNy1cWyozmgC5E-x_ekBWRXyJSiW3cpBss9sFpq638frUGA$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!RziG-qz2QLFAQZwBNy1cWyozmgC5E-x_ekBWRXyJSiW3cpBss9sFpq7_IKRR1w$

plexoos commented 2 years ago

Now I replaced my class ostrstring to St_ostrstring and added #define ostrstream St_ostrstream No clashes any more

First, I think you meant to say that you did s/ostrstream/St_ostrstream/ rather than s/ostrstring/St_ostrstring/ This is exactly what this PR is proposing to do across the entire code base.

Then, why do you think the #define ostrstream St_ostrstream is necessary?

Finally, if you look at the result of the CI checks (which you can access from this PR page) you see that the code with the above modification compiles fine but the test jobs in ROOT5 fail with the following message:

Error: operator<< not defined for St_ostrstream /star-sw/StRoot/macros/bfc.C:144:
Error: << Illegal operator for pointer 3 /star-sw/StRoot/macros/bfc.C:144:
*** Interpreter error recovered ***

So, clearly just the renaming of ostrstream to St_ostrstream is not enough to take care of this problem.

plexoos commented 2 years ago

I don't know if Jason is back from his vacation but he wanted to try something different https://github.com/star-bnl/star-sw/issues/126#issuecomment-907532000

I think the idea was to replace the STAR's ostream with std::ostringstream. If @klendathu2k can come up with a nice PR for this we can see if it works.

klendathu2k commented 2 years ago

Hi Dmitri,

I am back from vacation. I'll assemble a PR this weekend.

Jason

On 2021-09-03 16:51, Dmitri Smirnov wrote:

I don't know if Jason is back from his vacation but he wanted to try something different #126 (comment) [1]

I think the idea was to replace the STAR's ostream with std::ostringstream. If @klendathu2k [2] can come up with a nice PR for this we can see if it works.

-- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [3], or unsubscribe [4]. Triage notifications on the go with GitHub Mobile for iOS [5] or Android [6].

Links:

[1] https://github.com/star-bnl/star-sw/issues/126#issuecomment-907532000 [2] https://github.com/klendathu2k [3] https://github.com/star-bnl/star-sw/pull/134#issuecomment-912803394 [4] https://github.com/notifications/unsubscribe-auth/ANL4LVD36CGNV4DZSHTEYWDUAEYOJANCNFSM5DJTQBXQ [5] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!W5MxGCz-YylYPfkuwix6wRvWvBm4IzunZpuAyUf5P6hrE1q3r3YKJFeoQkRkBg0$ [6] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!W5MxGCz-YylYPfkuwix6wRvWvBm4IzunZpuAyUf5P6hrE1q3r3YKJFeolHmlp44$

perevbnlgov commented 2 years ago

s/ostrstream/St_ostrstream/ rather than s/ostrstring/St_ostrstring/ No, every there in the Star code is used ostrstream

This is exactly what this PR is proposing to do across the entire code base. I do not want to change the entire code. In addition ostringstream does not have method const char* str(). It has std::string str(). So you must change also all using of str() I do not think that at the end of STAR we need to change a lot of code. (obout 100+ lines)

Then, why do you think the #define ostrstream St_ostrstream is necessary? As I explained to avoid clash between fake ostrstream and the real one. Everywhere where is declared #include Sstrstream this #define will convert it into St_ostrstream and everywhere where there is no such include and hence no

define,

the official std::ostrstream will be used. As a result no clash.

Error: operator<< not defined for St_ostrstream /star-sw/StRoot/macros/bfc.C:144: Error: << Illegal operator for pointer 3 /star-sw/StRoot/macros/bfc.C:144: No this is fixed

Victor On 2021-09-03 16:44, Dmitri Smirnov wrote:

Now I replaced my class ostrstring to St_ostrstring and added

define ostrstream St_ostrstream

No clashes any more

First, I think you meant to say that you did s/ostrstream/St_ostrstream/ rather than s/ostrstring/St_ostrstring/ This is exactly what this PR is proposing to do across the entire code base.

Then, why do you think the #define ostrstream St_ostrstream is necessary?

Finally, if you look at the result of the CI checks (which you can access from this PR page) you see that the code with the above modification compiles fine but the test jobs in ROOT5 fail with the following message:

Error: operator<< not defined for St_ostrstream /star-sw/StRoot/macros/bfc.C:144: Error: << Illegal operator for pointer 3 /star-sw/StRoot/macros/bfc.C:144: Interpreter error recovered

So, clearly just the renaming of ostrstream to St_ostrstream is not enough to take care of this problem.

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-912800031 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7IJOUJGCJIILG34AI3UAEXTLANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!VaM8cRp2P6_h-3tq4UBB8IH9g7RGZVzMQpzQnpZdABL-Ppeww8MgaI4ileI-tA$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!VaM8cRp2P6_h-3tq4UBB8IH9g7RGZVzMQpzQnpZdABL-Ppeww8MgaI5MsCV6dg$

plexoos commented 2 years ago

I believe we still don't have a working solution for #126 among the existing PRs

perevbnlgov commented 2 years ago

Do you mean that my solution does not work? If so, please show me where I can see it.

On 2021-09-12 16:45, Dmitri Smirnov wrote:

I believe we still don't have a working solution for #126 [1] among the existing PRs

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. Triage notifications on the go with GitHub Mobile for iOS [4] or Android [5].

Links:

[1] https://github.com/star-bnl/star-sw/issues/126 [2] https://github.com/star-bnl/star-sw/pull/134#issuecomment-917707305 [3] https://github.com/notifications/unsubscribe-auth/ANQUL7PMTOUMBNNOXYTBV3DUBUGPJANCNFSM5DJTQBXQ [4] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!Vtk3w32ZYewXh9Rpc54EnL3lqs-MOsmp1Uri4ent-aayf3GUkbxDuLq9Lj63Nw$ [5] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!Vtk3w32ZYewXh9Rpc54EnL3lqs-MOsmp1Uri4ent-aayf3GUkbxDuLr2zXEWHQ$

plexoos commented 2 years ago

Do you mean that my solution does not work? If so, please show me where I can see it.

Did you create a pull request with your solution?

perevbnlgov commented 2 years ago

In my Star6 origin there is the class class St_ostrstream : public std::ostringstream {

But in the main Star6 repository class ostrstream : public std::ostringstream {

In my comments I have: In StMessage.h ostrstream replaced tp std::St_ostrstreamm, see Stsstreamm.h comments. In Stsstream.h ostrstream was replaced by std::St_ostrstream to avoyd clash with real ç and #define std::St_ostrstream ostrstream, to avoid clash with the real ostrstreamm

I did all the pull requests and I do not know why my updates were not propagated. But this github is so complicated that I can nor be sure in anything

Victor

On 2021-09-13 12:54, Dmitri Smirnov wrote:

Do you mean that my solution does not work? If so, please show me where I can see it.

Did you create a pull request with your solution?

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-918385083 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7I6JTNAZIZMVVBMEE3UBYUF7ANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!XOTAvjwcbSU5MbMgW2tn8rijCkHPLGWiO90JwtS6HmIJHssleo2XOnk3Ug99qA$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!XOTAvjwcbSU5MbMgW2tn8rijCkHPLGWiO90JwtS6HmIJHssleo2XOnnq-4GBIA$

plexoos commented 2 years ago

I do not know why my updates were not propagated.

We cannot merge changes if they cause errors in our builds. You did create a few PRs but none of them passed the test build successfully. For example, see errors here https://github.com/star-bnl/star-sw/pull/130/checks?check_run_id=3477975211

klendathu2k commented 2 years ago

Hi Victor,

Bottom of the pull request page... https://github.com/star-bnl/star-sw/pull/134 ... it doesn't pass tests...

Digging into the test, it looks like it fails when a bfc.C macro is loaded under ROOT5. That would break all production chains...

Cheers, Jason

On 2021-09-13 15:21, perevbnlgov wrote:

In my Star6 origin there is the class class St_ostrstream : public std::ostringstream {

But in the main Star6 repository class ostrstream : public std::ostringstream {

In my comments I have: In StMessage.h ostrstream replaced tp std::St_ostrstreamm, see Stsstreamm.h comments. In Stsstream.h ostrstream was replaced by std::St_ostrstream to avoyd clash with real ç and #define std::St_ostrstream ostrstream, to avoid clash with the real ostrstreamm

I did all the pull requests and I do not know why my updates were not propagated. But this github is so complicated that I can nor be sure in anything

Victor

On 2021-09-13 12:54, Dmitri Smirnov wrote:

Do you mean that my solution does not work? If so, please show me where I can see it.

Did you create a pull request with your solution?

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-918385083 [2]

https://github.com/notifications/unsubscribe-auth/ANQUL7I6JTNAZIZMVVBMEE3UBYUF7ANCNFSM5DJTQBXQ [3]

https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!XOTAvjwcbSU5MbMgW2tn8rijCkHPLGWiO90JwtS6HmIJHssleo2XOnk3Ug99qA$ [4]

https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!XOTAvjwcbSU5MbMgW2tn8rijCkHPLGWiO90JwtS6HmIJHssleo2XOnnq-4GBIA$

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-918503664 [2] https://github.com/notifications/unsubscribe-auth/ANL4LVG5WSVH6SOWDZNQEFDUBZFM3ANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!TpV6bhZVne4Dz3hXXL0Eb_rlNX8WF8YZL3a0LxmCN5SxRJu8oNocI_qWNtKCiZg$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!TpV6bhZVne4Dz3hXXL0Eb_rlNX8WF8YZL3a0LxmCN5SxRJu8oNocI_qWhx2zHW8$

perevbnlgov commented 2 years ago

Looking on compilation I see the problem with StShadowMaker and StarGeoetry. No one of them was modified by me. my compilation is successful.

Victor

On 2021-09-14 00:57, Dmitri Smirnov wrote:

I do not know why my updates were not propagated.

We cannot merge changes if they cause errors in our builds. You did create a few PRs but none of them passed the test build successfully. For example, see errors here https://github.com/star-bnl/star-sw/pull/130/checks?check_run_id=3477975211

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-918801794 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7OGQWWWQXAXSCRBYRTUB3I3HANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!SPEwqDtYq47HnpkU1IfixLFuR7BscoTBp8z5kcbZIru3U6nkCbUga6McFV-TGg$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!SPEwqDtYq47HnpkU1IfixLFuR7BscoTBp8z5kcbZIru3U6nkCbUga6N9Hdzqcg$

plexoos commented 2 years ago

Victor, Are you looking at this line? https://github.com/star-bnl/star-sw/pull/130/checks?check_run_id=3477975211#step:3:15375

`

21 ERROR: process "/bin/bash -c source /etc/profile && export Vc_DIR=/opt/software/linux-scientific7-x8664/gcc-4.8.5/vc-0.7.4-gqbhzu2x5u2dbe5tafxnl36xj5qbwov4 && cd /star-sw && cons +StarVMC/Geometry && cons %OnlTools %StRoot/StShadowMaker && find /star-sw/.$STAR_HOST_SYS -name *.o -exec rm '{}' \;" did not complete successfully: exit code: 2

`

You need to look at the lines prior to this one.

The problem is not with either StarVMC/Geometry or StShadowMaker which is excluded form the build

plexoos commented 2 years ago

I am not sure what you mean by "your" compilation. The CI jobs build the entire code base (with the exception of StShadowMaker)

Actually, you probably want to comment about the compilation problems in your PR #130 This PR #134 has a problem with ostrstream in run time

perevbnlgov commented 2 years ago

On 2021-09-15 13:09, Dmitri Smirnov wrote:

I am not sure what you mean by "your" compilation. The CI jobs build the entire code base (with the exception of StShadowMaker) It is very simple. I have full code in the local repository and it compiled without errors

Actually, you probably want to comment about the compilation problems in your PR #130 [1] This PR #134 [2] has a problem with ostrstream in run time

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [3], or unsubscribe [4]. Triage notifications on the go with GitHub Mobile for iOS [5] or Android [6].

Links:

[1] https://github.com/star-bnl/star-sw/pull/130 [2] https://github.com/star-bnl/star-sw/pull/134 [3] https://github.com/star-bnl/star-sw/pull/134#issuecomment-920208198 [4] https://github.com/notifications/unsubscribe-auth/ANQUL7NVQYIFOVEOY2LOY7TUCDHLNANCNFSM5DJTQBXQ [5] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!WEc2FfWkXQifCaCHjGR1w9Dow6Z3DbUMzJe2ZhTG8n7WFEl2ntTZw9ck8dgcmw$ [6] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!WEc2FfWkXQifCaCHjGR1w9Dow6Z3DbUMzJe2ZhTG8n7WFEl2ntTZw9d4Rbu_dg$

perevbnlgov commented 2 years ago

I found that the problem in StHbtMaker. It is out of my compilation. It is no used anymore. Last comment was 7 years ago.

On 2021-09-15 12:52, Dmitri Smirnov wrote:

Victor, Are you looking at this line? https://github.com/star-bnl/star-sw/pull/130/checks?check_run_id=3477975211#step:3:15375

21 ERROR: process "/bin/bash -c source /etc/profile && export

Vc_DIR=/opt/software/linux-scientific7-x8664/gcc-4.8.5/vc-0.7.4-gqbhzu2x5u2dbe5tafxnl36xj5qbwov4 && cd /star-sw && cons +StarVMC/Geometry && cons %OnlTools %StRoot/StShadowMaker && find /star-sw/.$STAR_HOST_SYS -name *.o -exec rm '{}' \;" did not complete successfully: exit code: 2

You need to look at the lines prior to this one.

The problem is not with either StarVMC/Geometry or StShadowMaker which is excluded form the build

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/134#issuecomment-920193444 [2] https://github.com/notifications/unsubscribe-auth/ANQUL7OXC6D5ZDZFXP6BLUDUCDFNPANCNFSM5DJTQBXQ [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&amp;mt=8&amp;pt=524675__;!!P4SdNyxKAPE!Stf-6KYJZylamd0zPWL3APri5Y9S5GJKVWjFfGa8CPOYHFpKDZjyKvqxYNgQmA$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&amp;referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!Stf-6KYJZylamd0zPWL3APri5Y9S5GJKVWjFfGa8CPOYHFpKDZjyKvrlpKtWJg$

plexoos commented 2 years ago

Superseded by #150