| Message ID | 20260115111630.1892890-3-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-01-15 11:16:30) > In order for ADL to find the function, it must be in the namespace of any of > its arguments. Previously, however, that was not the case, and it has only > really worked by accident and could be easily made to fail by introducing > other `operator<<` overloads. > > For example, a user of this function in `libcamera::ipa` would no longer > compile after introducing an `operator<<` into the `libcamera::ipa` > namespace as that would essentially hide this overload, and without ADL > it would not be found. I see - this was where all my pain was trying to add operator<< to Quantized! Thank you for digging through it! > So move the function into the `utils` namespace. > Looks very reasonable to me: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/base/utils.h | 6 +++--- > src/libcamera/base/utils.cpp | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index 6ce1ace43..fc4f5079c 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -423,10 +423,10 @@ scope_exit(EF) -> scope_exit<EF>; > > #endif /* __DOXYGEN__ */ > > -} /* namespace utils */ > - > #ifndef __DOXYGEN__ > -std::ostream &operator<<(std::ostream &os, const utils::Duration &d); > +std::ostream &operator<<(std::ostream &os, const Duration &d); > #endif > > +} /* namespace utils */ > + > } /* namespace libcamera */ > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index 2c3f2b7e6..42a516097 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -655,10 +655,8 @@ void ScopeExitActions::release() > actions_.clear(); > } > > -} /* namespace utils */ > - > #ifndef __DOXYGEN__ > -std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > +std::ostream &operator<<(std::ostream &os, const Duration &d) > { > std::ostringstream s; > > @@ -671,4 +669,6 @@ std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > } > #endif > > +} /* namespace utils */ > + > } /* namespace libcamera */ > -- > 2.52.0 >
Quoting Barnabás Pőcze (2026-01-15 11:16:30) > In order for ADL to find the function, it must be in the namespace of any of > its arguments. Previously, however, that was not the case, and it has only > really worked by accident and could be easily made to fail by introducing > other `operator<<` overloads. > > For example, a user of this function in `libcamera::ipa` would no longer > compile after introducing an `operator<<` into the `libcamera::ipa` > namespace as that would essentially hide this overload, and without ADL > it would not be found. > > So move the function into the `utils` namespace. > > Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- So with these three patches I can successfully apply the following to my Quantized series: diff --git a/src/ipa/libipa/quantized.cpp b/src/ipa/libipa/quantized.cpp index 06143a97ab3e..67b79ca7b865 100644 --- a/src/ipa/libipa/quantized.cpp +++ b/src/ipa/libipa/quantized.cpp @@ -123,7 +123,7 @@ namespace ipa { */ /** - * \fn std::ostream &Quantized::operator<<(std::ostream &out, const Quantized<Traits> &q) + * \fn std::ostream &operator<<(std::ostream &out, const Quantized<Traits> &q) * \brief Insert a text representation of a Quantized into an output stream * \param[in] out The output stream * \param[in] q The Quantized diff --git a/src/ipa/libipa/quantized.h b/src/ipa/libipa/quantized.h index 0b2f7148c821..b3ae3520b5d8 100644 --- a/src/ipa/libipa/quantized.h +++ b/src/ipa/libipa/quantized.h @@ -56,20 +56,21 @@ struct Quantized { return !(*this == other); } - friend std::ostream &operator<<(std::ostream &out, - const Quantized<Traits> &q) - { - out << "[" << utils::hex(q.quantized()) - << ":" << q.value() << "]"; - - return out; - } - private: QuantizedType quantized_; float value_; }; +template<typename Traits> +std::ostream &operator<<(std::ostream &out, + const Quantized<Traits> &q) +{ + out << "[" << utils::hex(q.quantized()) + << ":" << q.value() << "]"; + + return out; +} + } /* namespace ipa */ } /* namespace libcamera */ and put the operator<< back outside of the Quantized class (though still templated in the header). As this series isn't merged yet - I'll keep this out of the v6 posting, but could be moved on top or included in this series depending on what wins the race. -- Kieran
Hi Barnabás, Thank you for the patch. On Thu, Jan 15, 2026 at 12:16:30PM +0100, Barnabás Pőcze wrote: > In order for ADL to find the function, it must be in the namespace of any of > its arguments. Previously, however, that was not the case, and it has only > really worked by accident and could be easily made to fail by introducing > other `operator<<` overloads. > > For example, a user of this function in `libcamera::ipa` would no longer > compile after introducing an `operator<<` into the `libcamera::ipa` > namespace as that would essentially hide this overload, and without ADL > it would not be found. > > So move the function into the `utils` namespace. I understand we can't mix, but Would defining all our operator<<() in the libcamera namespace work too ? If so, is the approach in this patch preferred ? > Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/utils.h | 6 +++--- > src/libcamera/base/utils.cpp | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index 6ce1ace43..fc4f5079c 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -423,10 +423,10 @@ scope_exit(EF) -> scope_exit<EF>; > > #endif /* __DOXYGEN__ */ > > -} /* namespace utils */ > - > #ifndef __DOXYGEN__ > -std::ostream &operator<<(std::ostream &os, const utils::Duration &d); > +std::ostream &operator<<(std::ostream &os, const Duration &d); > #endif > > +} /* namespace utils */ > + > } /* namespace libcamera */ > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index 2c3f2b7e6..42a516097 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -655,10 +655,8 @@ void ScopeExitActions::release() > actions_.clear(); > } > > -} /* namespace utils */ > - > #ifndef __DOXYGEN__ > -std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > +std::ostream &operator<<(std::ostream &os, const Duration &d) > { > std::ostringstream s; > > @@ -671,4 +669,6 @@ std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > } > #endif > > +} /* namespace utils */ > + > } /* namespace libcamera */
2026. 01. 22. 2:22 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Thu, Jan 15, 2026 at 12:16:30PM +0100, Barnabás Pőcze wrote: >> In order for ADL to find the function, it must be in the namespace of any of >> its arguments. Previously, however, that was not the case, and it has only >> really worked by accident and could be easily made to fail by introducing >> other `operator<<` overloads. >> >> For example, a user of this function in `libcamera::ipa` would no longer >> compile after introducing an `operator<<` into the `libcamera::ipa` >> namespace as that would essentially hide this overload, and without ADL >> it would not be found. >> >> So move the function into the `utils` namespace. > > I understand we can't mix, but Would defining all our operator<<() in > the libcamera namespace work too ? If so, is the approach in this patch > preferred ? I think it would also solve it. But I can't really see a strong enough reason to deviate from the practice of using ADL for non-member `operator<<()`. So my preference is very much to rely on ADL and define these operators in the ADL-appropriate namespace. > >> Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> include/libcamera/base/utils.h | 6 +++--- >> src/libcamera/base/utils.cpp | 6 +++--- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h >> index 6ce1ace43..fc4f5079c 100644 >> --- a/include/libcamera/base/utils.h >> +++ b/include/libcamera/base/utils.h >> @@ -423,10 +423,10 @@ scope_exit(EF) -> scope_exit<EF>; >> >> #endif /* __DOXYGEN__ */ >> >> -} /* namespace utils */ >> - >> #ifndef __DOXYGEN__ >> -std::ostream &operator<<(std::ostream &os, const utils::Duration &d); >> +std::ostream &operator<<(std::ostream &os, const Duration &d); >> #endif >> >> +} /* namespace utils */ >> + >> } /* namespace libcamera */ >> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp >> index 2c3f2b7e6..42a516097 100644 >> --- a/src/libcamera/base/utils.cpp >> +++ b/src/libcamera/base/utils.cpp >> @@ -655,10 +655,8 @@ void ScopeExitActions::release() >> actions_.clear(); >> } >> >> -} /* namespace utils */ >> - >> #ifndef __DOXYGEN__ >> -std::ostream &operator<<(std::ostream &os, const utils::Duration &d) >> +std::ostream &operator<<(std::ostream &os, const Duration &d) >> { >> std::ostringstream s; >> >> @@ -671,4 +669,6 @@ std::ostream &operator<<(std::ostream &os, const utils::Duration &d) >> } >> #endif >> >> +} /* namespace utils */ >> + >> } /* namespace libcamera */ >
On Mon, Jan 26, 2026 at 01:04:36PM +0100, Barnabás Pőcze wrote: > 2026. 01. 22. 2:22 keltezéssel, Laurent Pinchart írta: > > On Thu, Jan 15, 2026 at 12:16:30PM +0100, Barnabás Pőcze wrote: > >> In order for ADL to find the function, it must be in the namespace of any of > >> its arguments. Previously, however, that was not the case, and it has only > >> really worked by accident and could be easily made to fail by introducing > >> other `operator<<` overloads. > >> > >> For example, a user of this function in `libcamera::ipa` would no longer > >> compile after introducing an `operator<<` into the `libcamera::ipa` > >> namespace as that would essentially hide this overload, and without ADL > >> it would not be found. > >> > >> So move the function into the `utils` namespace. > > > > I understand we can't mix, but Would defining all our operator<<() in > > the libcamera namespace work too ? If so, is the approach in this patch > > preferred ? > > I think it would also solve it. But I can't really see a strong enough reason > to deviate from the practice of using ADL for non-member `operator<<()`. > > So my preference is very much to rely on ADL and define these operators in > the ADL-appropriate namespace. Works for me. > >> Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> --- > >> include/libcamera/base/utils.h | 6 +++--- > >> src/libcamera/base/utils.cpp | 6 +++--- > >> 2 files changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > >> index 6ce1ace43..fc4f5079c 100644 > >> --- a/include/libcamera/base/utils.h > >> +++ b/include/libcamera/base/utils.h > >> @@ -423,10 +423,10 @@ scope_exit(EF) -> scope_exit<EF>; > >> > >> #endif /* __DOXYGEN__ */ > >> > >> -} /* namespace utils */ > >> - > >> #ifndef __DOXYGEN__ > >> -std::ostream &operator<<(std::ostream &os, const utils::Duration &d); > >> +std::ostream &operator<<(std::ostream &os, const Duration &d); > >> #endif > >> > >> +} /* namespace utils */ > >> + > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > >> index 2c3f2b7e6..42a516097 100644 > >> --- a/src/libcamera/base/utils.cpp > >> +++ b/src/libcamera/base/utils.cpp > >> @@ -655,10 +655,8 @@ void ScopeExitActions::release() > >> actions_.clear(); > >> } > >> > >> -} /* namespace utils */ > >> - > >> #ifndef __DOXYGEN__ > >> -std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > >> +std::ostream &operator<<(std::ostream &os, const Duration &d) > >> { > >> std::ostringstream s; > >> > >> @@ -671,4 +669,6 @@ std::ostream &operator<<(std::ostream &os, const utils::Duration &d) > >> } > >> #endif > >> > >> +} /* namespace utils */ > >> + > >> } /* namespace libcamera */
diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h index 6ce1ace43..fc4f5079c 100644 --- a/include/libcamera/base/utils.h +++ b/include/libcamera/base/utils.h @@ -423,10 +423,10 @@ scope_exit(EF) -> scope_exit<EF>; #endif /* __DOXYGEN__ */ -} /* namespace utils */ - #ifndef __DOXYGEN__ -std::ostream &operator<<(std::ostream &os, const utils::Duration &d); +std::ostream &operator<<(std::ostream &os, const Duration &d); #endif +} /* namespace utils */ + } /* namespace libcamera */ diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp index 2c3f2b7e6..42a516097 100644 --- a/src/libcamera/base/utils.cpp +++ b/src/libcamera/base/utils.cpp @@ -655,10 +655,8 @@ void ScopeExitActions::release() actions_.clear(); } -} /* namespace utils */ - #ifndef __DOXYGEN__ -std::ostream &operator<<(std::ostream &os, const utils::Duration &d) +std::ostream &operator<<(std::ostream &os, const Duration &d) { std::ostringstream s; @@ -671,4 +669,6 @@ std::ostream &operator<<(std::ostream &os, const utils::Duration &d) } #endif +} /* namespace utils */ + } /* namespace libcamera */
In order for ADL to find the function, it must be in the namespace of any of its arguments. Previously, however, that was not the case, and it has only really worked by accident and could be easily made to fail by introducing other `operator<<` overloads. For example, a user of this function in `libcamera::ipa` would no longer compile after introducing an `operator<<` into the `libcamera::ipa` namespace as that would essentially hide this overload, and without ADL it would not be found. So move the function into the `utils` namespace. Fixes: 5055ca747c4c ("libcamera: utils: Add helper class for std::chrono::duration") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/base/utils.h | 6 +++--- src/libcamera/base/utils.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)