Districts : simplifying the code #2279

guilpier-code · 2024-07-22T15:18:14Z

Type SetsOfAreas (see study.h) is used to print aggregated results for a group of areas.
Its friend SetsOfLinks is defined but never used.
We remove it and make subsequent simplifications :

class Sets no longer needs to be a template class, or have template methods (So sets.hxx is removed, and sets.cpp is created)
class SetHandlerAreas (specific operations very much related to sets of areas) needs to be moved from Study code area into sets.h/.cpp

What should be done in the future :

class SetHandlerAreas should be questioned as a class : does it constitute a cohesive code entity ? Should it not be integrated to class Sets ? What should we do with its code so that Sets are clearer and simpler (next point may give a solution for this point) ?
class Sets should be refactored : indeed, it contains many private std::vectors / std::maps (pMap, pOptions, pByIndex, ...), associating a property to a set of area. We make many loops over these containers. This is chaotic design.
Instead, we should define a class Set (for a single set), and make existing class Sets a collection of Set.
Properties of a set should be contained in class Set. Besides the fact it would be more natural and clear, I suspect it would cause many simplifications and code reductions.

This reverts commit 065acfb.

We create a shared_ptr on the stack, stock the raw pointer in a vector then exit the scop. At this point the pointer is not valid anymore

Now that type setsOfLinks is gone (see previous commit), template about class Sets is useless. We remove it to simplify code.

src/libs/antares/study/include/antares/study/sets.h

src/libs/antares/study/include/antares/study/sets.hxx

…Sets)

flomnes

Minor remarks.

typename makes no sense if there is no template, use auto instead
Use modern style for loops

src/libs/antares/study/area/sets.cpp

flomnes · 2024-07-25T13:19:21Z

src/libs/antares/study/area/sets.cpp

+    const typename MapType::const_iterator end = pMap.end();
+    for (typename MapType::const_iterator i = pMap.begin(); i != end; ++i)


Suggested change

const typename MapType::const_iterator end = pMap.end();

for (typename MapType::const_iterator i = pMap.begin(); i != end; ++i)

for (const auto& [name, content] : pMap)

I'll do that, no problem, but we should keep in mind that any std::map / std::vector used as a data members of class Sets (for instance pMap) should not exist : please see PR presentation comment.
std::map and std::vector are used because of a chaotic architecture of class Sets.
Making changes on these std::map and std::vector manipulations is making changes on things that should disappear.

src/libs/antares/study/area/sets.cpp

… removed)

guilpier-code · 2024-07-30T14:40:44Z

typename makes no sense if there is no template, use auto instead

Done : all typenames were removed from sets.h/.cpp

…a for loop + clang format

src/libs/antares/study/area/sets.cpp

flomnes · 2024-07-31T09:32:06Z

Add unit / study tests

discussion

…for review

sonarqubecloud · 2024-08-21T16:03:32Z

Please retry analysis of this Pull-Request directly on SonarCloud

…fy-districts

…lex tests on CI

guilpier-code · 2024-08-22T12:58:19Z

Add unit / study tests

I added many tests to be run on CI (see this commit), knowing that district results are taken into account when running CI tests.

flomnes

Minor remarks

Revert workflow files
Use emplace_back(args...) instead of push_back(T(args...))

flomnes · 2024-08-23T12:35:34Z

src/libs/antares/study/area/sets.cpp

-    opts.rules.push_back(Rule(ruleFilter, new String("add-all")));
-    auto item = std::make_shared<T>();
-    add("all areas", item, opts);
+    opts.rules.push_back(Rule(ruleFilter, "add-all"));


Suggested change

opts.rules.push_back(Rule(ruleFilter, "add-all"));

opts.rules.emplace_back(ruleFilter, "add-all");

Construction in place. I get it.

flomnes · 2024-08-23T12:36:40Z

src/libs/antares/study/area/sets.cpp

@@ -239,17 +209,17 @@ bool Sets<T>::loadFromFile(const std::filesystem::path& filename)

                if (p->key == "+")
                {
-                    opts.rules.push_back(Rule(ruleAdd, new String(value)));
+                    opts.rules.push_back(Rule(ruleAdd, value.to<std::string>()));


This is so much better, the previous code was very confusing regarding memory ownership

flomnes · 2024-08-23T12:40:51Z

.github/workflows/ubuntu.yml

Revert this

flomnes · 2024-08-23T12:40:56Z

.github/workflows/windows-vcpkg.yml

guilpier-code · 2024-08-23T13:48:28Z

Add unit / study tests

Unit tests could be added easier by doing recommended refactoring (see PR presentation)

sonarqubecloud · 2024-08-23T17:31:08Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
44.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

payetvin and others added 23 commits July 18, 2024 15:23

sets byIndex

6015a25

sets namebyIndex

67ba855

correlation montlhy

4bdf895

correlation annual

404c817

small refactor in correlation

680cb2e

atsp tmparray

2da0e70

scenario rules

05aaac0

xcast in ts generator

065acfb

Revert "xcast in ts generator"

7438946

This reverts commit 065acfb.

use make_shared for sets

fc5d298

Merge branch 'develop' into feature/remove-new

978f04b

link info in cbuilder

b9bf04a

thermal cluster prepro

7b6310c

format

b103fd7

Use unique_ptr

7673fe9

Remove this

83ec498

Use unique_ptr

b1cc714

Remove dynamic allocations (#2275)

ed80dae

Simplify districts : remove useless setsOfLinks.

d1c32f4

Use uniqie_ptr

98169b6

Fix build

ec17e72

Fix potential access violation (#2278)

58de3f2

We create a shared_ptr on the stack, stock the raw pointer in a vector then exit the scop. At this point the pointer is not valid anymore

Simplify districts : remove useless templates of class Sets

48a2246

Now that type setsOfLinks is gone (see previous commit), template about class Sets is useless. We remove it to simplify code.

guilpier-code requested a review from flomnes July 22, 2024 15:18

pull-request-size bot added the size/XL label Jul 22, 2024

guilpier-code changed the base branch from develop to feature/remove-new July 22, 2024 15:19

Merge branch 'feature/remove-new' into fix/simplify-districts

597177c

flomnes linked an issue Jul 22, 2024 that may be closed by this pull request

Many .hxx files #2276

Closed

guilpier-code commented Jul 23, 2024

View reviewed changes

src/libs/antares/study/include/antares/study/sets.h Show resolved Hide resolved

src/libs/antares/study/include/antares/study/sets.h Show resolved Hide resolved

src/libs/antares/study/include/antares/study/sets.hxx Show resolved Hide resolved

Simplify districts : move set handler were it belongs (from Study to …

3b2e9c6

…Sets)

pull-request-size bot added the size/XL label Jul 24, 2024

Simplify districts : formatting

308ea5e

guilpier-code marked this pull request as ready for review July 25, 2024 11:10

flomnes approved these changes Jul 25, 2024

View reviewed changes

flomnes reviewed Jul 25, 2024

View reviewed changes

Simplify districts : removing useless "typename"s (now templates were…

8977058

… removed)

guilpier-code added 2 commits July 30, 2024 18:28

Simplify districts : use more modern loops over STL containers

0237365

Simplify districts : make Sets::add(...) functions private + improve …

229e733

…a for loop + clang format

flomnes reviewed Jul 31, 2024

View reviewed changes

src/libs/antares/study/area/sets.cpp Outdated Show resolved Hide resolved

src/libs/antares/study/area/sets.cpp Outdated Show resolved Hide resolved

src/libs/antares/study/area/sets.cpp Outdated Show resolved Hide resolved

Simplify districts : changes die to review

b99d7b7

flomnes previously approved these changes Jul 31, 2024

View reviewed changes

JasonMarechal25 and others added 3 commits August 19, 2024 13:18

Merge branch 'develop' into fix/simplify-districts

9189793

Simplify districts : just reorder class Sets' methods to look better …

ca613bf

…for review

Simplify districts : apply clang format

14b0e7a

guilpier-code added 2 commits August 22, 2024 10:49

Merge remote-tracking branch 'remotes/origin/develop' into fix/simpli…

ab3ac26

…fy-districts

Simplify districts : temporarily activate running of all tests + comp…

cf8fa43

…lex tests on CI

guilpier-code requested a review from flomnes August 22, 2024 14:15

flomnes reviewed Aug 23, 2024

View reviewed changes

Simplify districts : replace push_back with emplace_back

842d1d4

flomnes approved these changes Aug 23, 2024

View reviewed changes

Revert workflows

a92a56f

flomnes merged commit 6e26d9c into develop Aug 23, 2024
6 of 7 checks passed

flomnes deleted the fix/simplify-districts branch August 23, 2024 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Districts : simplifying the code #2279

Districts : simplifying the code #2279

guilpier-code commented Jul 22, 2024 •

edited

Loading

flomnes left a comment

flomnes Jul 25, 2024

guilpier-code Jul 30, 2024 •

edited

Loading

guilpier-code Aug 22, 2024

guilpier-code commented Jul 30, 2024

flomnes commented Jul 31, 2024

sonarqubecloud bot commented Aug 21, 2024

guilpier-code commented Aug 22, 2024 •

edited

Loading

flomnes left a comment

flomnes Aug 23, 2024

guilpier-code Aug 23, 2024

flomnes Aug 23, 2024

flomnes Aug 23, 2024

flomnes Aug 23, 2024

guilpier-code commented Aug 23, 2024

sonarqubecloud bot commented Aug 23, 2024

		const typename MapType::const_iterator end = pMap.end();
		for (typename MapType::const_iterator i = pMap.begin(); i != end; ++i)

	const typename MapType::const_iterator end = pMap.end();
	for (typename MapType::const_iterator i = pMap.begin(); i != end; ++i)
	for (const auto& [name, content] : pMap)

	opts.rules.push_back(Rule(ruleFilter, "add-all"));
	opts.rules.emplace_back(ruleFilter, "add-all");

Districts : simplifying the code #2279

Districts : simplifying the code #2279

Conversation

guilpier-code commented Jul 22, 2024 • edited Loading

flomnes left a comment

Choose a reason for hiding this comment

flomnes Jul 25, 2024

Choose a reason for hiding this comment

guilpier-code Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

guilpier-code Aug 22, 2024

Choose a reason for hiding this comment

guilpier-code commented Jul 30, 2024

flomnes commented Jul 31, 2024

sonarqubecloud bot commented Aug 21, 2024

guilpier-code commented Aug 22, 2024 • edited Loading

flomnes left a comment

Choose a reason for hiding this comment

flomnes Aug 23, 2024

Choose a reason for hiding this comment

guilpier-code Aug 23, 2024

Choose a reason for hiding this comment

flomnes Aug 23, 2024

Choose a reason for hiding this comment

flomnes Aug 23, 2024

Choose a reason for hiding this comment

flomnes Aug 23, 2024

Choose a reason for hiding this comment

guilpier-code commented Aug 23, 2024

sonarqubecloud bot commented Aug 23, 2024

Quality Gate passed

guilpier-code commented Jul 22, 2024 •

edited

Loading

guilpier-code Jul 30, 2024 •

edited

Loading

guilpier-code commented Aug 22, 2024 •

edited

Loading