Skip to content
Snippets Groups Projects

WIP: 22.0-cleanup-SiClusterizationTool

Closed Shaun Roe requested to merge sroe/athena:22.0-cleanup-SiClusterizationTool into master
All threads resolved!

Primarily improve error reporting so use of at() with an index out of range is trapped and reports a meaningful ATH_MSG_FATAL message instead of creating an exception (in context of ATLIDTRKCP-302). Other cleanups done: use of cmath constants instead of TMath, tidy up includes, use of std::array instead of a c style array, clarification of the declaration and usage of a pointer-to-member-function variable.

Change to WIP while urgent changes go in (!38018 (merged))

Edited by Shaun Roe

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Resolved by Shaun Roe

      Thanks @sroe, this is a really nice clean up. In addition to my one question above I had a couple of more general comments

      • There are places where the indentation looks to have gone a bit off (particularly around log messages). This may well be deliberate but if not I thought I'd let you know.
      • I don't think the ATLAS c++ style guide says anything specifically about whether to use the word or symbol form of logical operators, but I think it would be good to be consistent within a single file (and vs &&, not vs !, or vs ||)

      Tom - L1 shifter

      Edited by Thomas James Neep
  • In fact, since you are one of the authors of the style guide perhaps you have a view on the logical operator thing?

  • Shaun Roe added 1 commit

    added 1 commit

    • 0490699a - Update NnClusterizationFactory.cxx

    Compare with previous version

  • This merge request affects 2 files:

    • InnerDetector/InDetRecTools/SiClusterizationTool/SiClusterizationTool/NnClusterizationFactory.h
    • InnerDetector/InDetRecTools/SiClusterizationTool/src/NnClusterizationFactory.cxx

    Adding @goetz ,@amorley ,@oda ,@sroe as watchers

  • Shaun Roe resolved all threads

    resolved all threads

  • Shaun Roe mentioned in merge request !38018 (merged)

    mentioned in merge request !38018 (merged)

  • CI Result SUCCESS (hash 0490699a)

    Athena AthSimulation AthGeneration AnalysisBase
    externals
    cmake
    make
    required tests
    optional tests

    Full details available on this CI monitor view
    Athena: number of compilation errors 0, warnings 0
    AthSimulation: number of compilation errors 0, warnings 0
    AthGeneration: number of compilation errors 0, warnings 0
    AnalysisBase: number of compilation errors 0, warnings 0
    📝 For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 23157]

  • Shaun Roe marked as a Work In Progress

    marked as a Work In Progress

  • Shaun Roe changed the description

    changed the description

  • @sroe , sorry for not merging this one earlier. Yesterday I had to put a few merge requests on hold because we were preparing good nightly for 22.0.20.

    Meanwhile we got merge conflicts on this one. Could you please fix them? Thanks.

  • Author Developer

    Hi @tsulaia , in fact I intended this to be delayed once I saw that Gabriel F. and Dan G. had urgent fixes to go in. I will see what I can do once all their MRs are in; possibly their changes have made this one redundant.

  • closed

  • Shaun Roe mentioned in merge request !38231 (merged)

    mentioned in merge request !38231 (merged)

  • Please register or sign in to reply
    Loading