Skip to content

Conversation

@plexoos
Copy link
Member

@plexoos plexoos commented Sep 10, 2025

The lines come directly form /afs/rhic.bnl.gov/star/ROOT/5.34.38/root/etc/rootlogon.C

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds optional floating point exception (FPE) trapping functionality to the ROOT logon script. The implementation checks environment variables to determine whether FPE trapping should be enabled, with automatic activation for development versions.

Key changes:

  • Adds conditional FPE trapping based on STAR_VERSION and STARFPE environment variables
  • Provides console feedback on FPE status
  • Uses a namespace to organize the FPE-related variables

{
// set FloatPointException trap
namespace rootlogon {
int fpe=0;const char *env=0;
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple variable declarations on a single line reduce readability. Each variable should be declared on its own line for better maintainability.

Suggested change
int fpe=0;const char *env=0;
int fpe = 0;
const char *env = 0;

Copilot uses AI. Check for mistakes.
gSystem->SetFPEMask(kInvalid | kDivByZero | kOverflow );
printf("*** Float Point Exception is ON ***\n");
} else {
printf("*** Float Point Exception is OFF ***\n");
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term 'Float Point Exception' should be 'Floating Point Exception' to use the correct spelling.

Copilot uses AI. Check for mistakes.
@plexoos plexoos requested review from fgeurts and removed request for R-Witt and iraklic September 10, 2025 18:00
@genevb
Copy link
Contributor

genevb commented Sep 10, 2025

@plexoos , are you intending to re-commit each change introduced into rootlogon.C documented by the CVS commits from 1998-2016 that aren't already in the version re-introduced in 2021? The CVS commit comments do already provide some minimal documentation of the added lines.

https://www.star.bnl.gov/cgi-bin/protected/viewvc.cgi/cvsroot/StRoot/macros/rootlogon.C?hideattic=0&view=log
and
https://www.star.bnl.gov/cgi-bin/protected/viewvc.cgi/cvsroot/root5/etc/rootlogon.C?hideattic=0&view=log

@plexoos
Copy link
Member Author

plexoos commented Sep 10, 2025

are you intending to re-commit each change introduced into rootlogon.C documented by the CVS commits from 1998-2016 that aren't already in the version re-introduced in 2021?

No, only the ones we discussed: fpe and xrootd

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As further information, this is a restoration of prior commits to the rootlogon.C file, 98a061e and 53b15b0 .

@plexoos plexoos force-pushed the restore-rootlogon-fpe branch from 4bfc15c to 7ba1d45 Compare September 16, 2025 14:23
@plexoos plexoos mentioned this pull request Sep 17, 2025
Copy link
Contributor

@klendathu2k klendathu2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@plexoos plexoos merged commit da7ee0d into main Sep 17, 2025
148 checks passed
@plexoos plexoos deleted the restore-rootlogon-fpe branch September 17, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants