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

Aos 2 metadata traits decorators efif #8592

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

efifogel
Copy link
Member

@efifogel efifogel commented Nov 6, 2024

Summary of Changes

Introduces two traits decorators, namely Arr_tracing_traits_2 and Arr_counting_traits_2, which can be used to extract debugging and informative metadata about the traits in use while the program is being executed. They have been part of the CGAL distribution for several years now . (Probably more than a decade.) I gave it a face lift, wrote the necessary documentation, and added a small example program called count_and_trace.cpp.

Release Management

  • Affected package(s): 2D Arrangements (Arrangement_on_surface_2)
  • Issue(s) solved (if any): NA
  • Feature/Small Feature (if any): Aos 2 Metadata Traits Decorators -- Pre-approval date 2024/11/12
  • Link to compiled documentation (obligatory for small feature) Manual
  • License and copyright ownership: TAU

Copy link
Contributor

@albert-github albert-github left a comment

Choose a reason for hiding this comment

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

I just read quickly through the file, same problems might exist in other files.


/*! \ingroup PkgArrangementOnSurface2TraitsClasses
*
* A meradata traits-class decorator for the arrangement package. It counts the
Copy link
Contributor

Choose a reason for hiding this comment

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

meradata -> metadata ?

* another traits class and inherits from it. For each traits method it
* maintains a counter that counts the number of invocations into the method.
*
* It models all the concept that the original trais models.
Copy link
Contributor

Choose a reason for hiding this comment

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

trais -> traits?
concept -> concepts?

Comment on lines 73 to 74
/*! Disable copy constructor.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

one line?

/*! Obtain the counter of the given operation */
std::size_t count(Operation_id id) const;

/*! Print the compare_x counter */
Copy link
Contributor

Choose a reason for hiding this comment

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

compare_x ?

@efifogel
Copy link
Member Author

efifogel commented Nov 7, 2024 via email

@afabri
Copy link
Member

afabri commented Nov 8, 2024

trailing whitespace

Copy link
Contributor

@albert-github albert-github left a comment

Choose a reason for hiding this comment

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

Maybe also run something like /build:v0 to create the documentation so you can check it.

debugging and informative metadata to an output stream. The former is
used to count the number of invocations of traits-class functors, and
the latter is used to trace these invocations. Note that the
constructors of each of these calls templates applies prefect
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit a strange sentence

*/
void disable_all_traces();

/// \name Types and functors inherited from the base
Copy link
Contributor

Choose a reason for hiding this comment

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

base of what, class, traits, ...?

* \param p the exact point.
* \param i the coordinate index (either 0 or 1).
* \pre i is either 0 or 1.
* \return An approximation of p's \f$x\f$-coordinate (if i == 0), or an
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't if i==0 no be in a code font?
(same for if i==1)

@@ -727,18 +667,18 @@ class Arr_counting_traits_2 : public Base_traits {
{ ++m_counter3; return m_object(xcv1, ce1, xcv2, ce2); }
};

/*! A functor that compares the x-coordinates of curve ends near the
/*! A functor that compares the \f$x\f$-coordinates of curve ends near the
Copy link
Contributor

Choose a reason for hiding this comment

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

here the x is shown as a formula but at other places the y of y-coordinate is not shown as a formula ...

Comment on lines 795 to 796
* \return An approximation of p's \f$x\f$-coordinate (if i == 0), or an
* approximation of p's y-coordinate (if i == 1).
Copy link
Contributor

Choose a reason for hiding this comment

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

p and if ... in code format?

debugging and informative metadata to an output stream. The former is
used to count the number of invocations of traits-class functors, and
the latter is used to trace these invocations. Note that the
constructors of each of these calls templates applies prefect
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constructors of each of these calls templates applies prefect
constructors of each of these calls templates applies perfect

@efifogel
Copy link
Member Author

efifogel commented Nov 10, 2024 via email

debugging and informative metadata to an output stream. The former is
used to count the number of invocations of traits-class functors, and
the latter is used to trace these invocations. Note that the
constructors of each of these class templates applies perfect
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constructors of each of these class templates applies perfect
constructors of each of these class templates apply perfect

`Arr_tracing_traits_2::MAKE_X_MONOTONE_OP`. The calls
`traits.enable_all_traces()` and `traits.disable_all_traces()` enables
and disables the traces of all functors, respectively. The example
program listed below and coded in the file `count_and_trace.cpp`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
program listed below and coded in the file `count_and_trace.cpp`
program listed below

/// \name Creation
/// @{

/*! Construct default */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Construct default */
/*! constructs default */

template <typename ... Args>
Arr_counting_traits_2(Args ... args) : Base(std::forward<Args>(args)...) {}

/*! Disable copy constructor. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Disable copy constructor. */
/*! disables copy constructor. */


/// @}

/*! Obtain the counter of the given operation */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Obtain the counter of the given operation */
/*! obtains the counter of the given operation */

@@ -374,7 +367,7 @@ class Arr_circle_segment_traits_2 {
}
};

/*! Get an Equal_2 functor object. */
/*! Obtain an `Equal_2` functor object. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Obtain an `Equal_2` functor object. */
/*! obtains an `Equal_2` functor object. */

@@ -554,15 +547,15 @@ class Arr_circle_segment_traits_2 {
}
};

/*! Obtain an Approximate_2 functor object. */
/*! Obtain an `Approximate_2` functor object. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Obtain an `Approximate_2` functor object. */
/*! obtains an `Approximate_2` functor object. */

@@ -573,8 +566,8 @@ class Arr_circle_segment_traits_2 {
public:
Make_x_monotone_2(bool use_cache = false) : m_use_cache(use_cache) {}

/*! Subdivide a given circular arc or line segment into x-monotone subcurves
* and insert them to a given output iterator.
/*! Subdivide a given circular arc or line segment into \f$x\f$-monotone
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Subdivide a given circular arc or line segment into \f$x\f$-monotone
/*! subdivides a given circular arc or line segment into \f$x\f$-monotone

Make_x_monotone_2 make_x_monotone_2_object() const
{ return Make_x_monotone_2(m_use_cache); }

class Split_2
{
public:

/*!
* Split a given x-monotone curve at a given point into two sub-curves.
/*! Split a given \f$x\f$-monotone curve at a given point into two
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Split a given \f$x\f$-monotone curve at a given point into two
/*! splits a given \f$x\f$-monotone curve at a given point into two

@@ -707,7 +700,7 @@ class Arr_circle_segment_traits_2 {
}
};

/*! Get a Split_2 functor object. */
/*! Obtain a `Split_2` functor object. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/*! Obtain a `Split_2` functor object. */
/*! obtains a `Split_2` functor object. */

Copy link
Member

Choose a reason for hiding this comment

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

I stop here but you get the idea

@sloriot sloriot added Enhancement Pkg::Arrangement_on_surface_2 pre-approved For pre-approved small features. After 15 days the feature will be accepted. Under Testing labels Nov 12, 2024
@efifogel
Copy link
Member Author

efifogel commented Nov 13, 2024 via email

/*! Enable the trace of a traits operation
* \param id the operation identifier
/*! enables the trace of a traits operation.
* \param id the operation identifier.
*/
void enable_trace(Operation_id id) { m_flags |= 0x1 << id; }
Copy link
Member

Choose a reason for hiding this comment

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

89>C:\CGAL_ROOT\CGAL-6.1-Ic-19\include\CGAL\Arr_tracing_traits_2.h(179,55): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\CGAL_ROOT\CGAL-6.1-Ic-19\cmake\platforms\MSVC-2022-Community-Release\test\Arrangement_on_surface_2_Examples\count_and_trace.vcxproj] not sure if it's a new warning

@efifogel
Copy link
Member Author

efifogel commented Nov 14, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Pkg::Arrangement_on_surface_2 pre-approved For pre-approved small features. After 15 days the feature will be accepted. Under Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants