Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved number-related files into separate directory #805

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

Tomaqa
Copy link
Member

@Tomaqa Tomaqa commented Nov 6, 2024

I am making changes in the handling of numbers according to PR #751.
As a first step, it will be useful to separate this kind of files into a separate directory.
I do not want to make these renaming changes in one PR because keeping track of the actual modifications would be difficult then.

#include <common/InternalException.h>
#include <common/StringConv.h>
#include <common/TreeOps.h>
#include <common/numbers/FastRational.h>
#include <common/numbers/Number.h>
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need to include the same file both in .h and the .cc file?
If it is included in the header, I would prefer to not include it again the the implementation file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove Number.h.
FastRational.h is not necessarily included by ArithLogic.h if FAST_RATIONALS is not defined, but since it is hardcoded this way it would not work in the other case anyway.
So I will remove it as well ...

Copy link
Member

Choose a reason for hiding this comment

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

I only meant Number.h

#include <common/NumberUtils.h>
#include <common/numbers/NumberUtils.h>

#include <gmpxx.h>
Copy link
Member

Choose a reason for hiding this comment

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

Why this new header?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not remember. Seems redundant. Removing.

#include "LIAInterpolator.h"
#include "CutCreator.h"

#include <models/ModelBuilder.h>
#include <common/numbers/Real.h>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this unnecessary?

Copy link
Member Author

@Tomaqa Tomaqa Nov 6, 2024

Choose a reason for hiding this comment

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

I think not because it is not clear where Real comes from. I already experienced a few times that if not included explicitly, and if the order of the other included headers changes e.g. during a refactoring, the compilation may fail and even only on some platforms. Furthermore, it was not trivial to track down such compilation errors.
Hence I consider a better practice to not always rely on indirect inclusions by other headers. Here it would be probably better to move the inclusion into LASolver.h, though.

@@ -5,6 +5,7 @@
#include "Simplex.h"

#include <common/InternalException.h>
#include <common/numbers/Number.h>
Copy link
Member

Choose a reason for hiding this comment

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

Also seems unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should actually be Real.h, and moving it to Simplex.h

Copy link
Member

@blishko blishko left a comment

Choose a reason for hiding this comment

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

OK!

@Tomaqa Tomaqa merged commit e8eb84b into master Nov 6, 2024
9 checks passed
@blishko blishko deleted the numbers branch November 6, 2024 18:42
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.

2 participants