Introduce official clang-format

[lpranam]

Description

provides .clang-format with the latest(clang-format 12) configuration

References

• Closes: Unified formatting through clang-format #518
• Brainstorm .clang-format proposal #87
• example/clang-format

Tasklist

• Add test case(s)
• Ensure all CI builds pass
• Review and approve

[lpranam]

It is manually configured according to what seemed more readable to me.

not so useful options or non cpp options are left as default by llvm style

[mloskot]

First, I'm happy to see this initiative moving forward.

How this configuration relates to the initial one and example linked from here #518 (comment) ? Is this significantly different?
I'd like to see some reference to those in this PR.

[lpranam]

the example provided in #518 is default LLVM while in this PR it follows our existing example at most of the places and I used all the options so that formatting can be fine-tuned and better. and also some twiques to make this more suitable for us i.e: IncludeCategories

[mloskot]

Right, but the #87 one offers customised configuration. I was mostly interested to know how different is this one from the #87 I have not had a chance to try it out myself. I may have time on Wednesday.

[lpranam]

I have not tested this against #87. but I have tested this against our example file(i don't know if they are same), only difference in there(from the few files I tested)

1. if parameters do not fit one parameter each line
2. align the assignment operators

My main test files were kernel.hpp and convolve.hpp

[mloskot]

Do you know if there is any way to tell clang-format to respect custom macro like BOOST_FORCEINLINE as inline?

Currently, this:

BOOST_FORCEINLINE
int foo() { return 1 + 2; }

inline
int bar() { return 1 + 2; }

becomes this

BOOST_FORCEINLINE int
    foo()
{
    return 1 + 2;
}

inline int bar()
{
    return 1 + 2;
}

---

UPDATE: I can not find any option for inline. It looks like it's not possible, neither for static.
Then, I think, it is even more important to refactor into trailing return as I explained in #596 (comment)

[mloskot]

Do you know if there is any way in clang-format 12 to handle the trailing return better way?

Here is one bit from algorithm.hpp which I manually formatted long time ago

template<typename T, typename CS>
auto copy(
    boost::gil::pixel<T, CS>* first,
    boost::gil::pixel<T, CS>* last,
    boost::gil::pixel<T, CS>* dst)
    ->  boost::gil::pixel<T, CS>*
{
    auto p = std::copy((unsigned char*)first, (unsigned char*)last, (unsigned char*)dst);
    return reinterpret_cast<boost::gil::pixel<T, CS>*>(p);
}

and clang-format formats it this way:

template <typename T, typename CS>
auto copy(
    boost::gil::pixel<T, CS>* first,
    boost::gil::pixel<T, CS>* last,
    boost::gil::pixel<T, CS>* dst) -> boost::gil::pixel<T, CS>*
{
    auto p = std::copy((unsigned char*)first, (unsigned char*)last, (unsigned char*)dst);
    return reinterpret_cast<boost::gil::pixel<T, CS>*>(p);
}

I like the former. Or, we can just live with the latter :-)

[mloskot]

I think this would also be an opportunity to refactor functions into trailing return type.

For example, this input

template <typename IC>
inline typename type_from_x_iterator<planar_pixel_iterator<IC,cmyk_t> >::view_t
planar_cmyk_view(std::size_t width, std::size_t height, IC c, IC m, IC y, IC k, std::ptrdiff_t rowsize_in_bytes)
{
    using view_t = typename type_from_x_iterator<planar_pixel_iterator<IC,cmyk_t> >::view_t;
    return view_t(width, height, typename view_t::locator(planar_pixel_iterator<IC,cmyk_t>(c,m,y,k), rowsize_in_bytes));
}

is reformatted as

template <typename IC>
inline typename type_from_x_iterator<planar_pixel_iterator<IC, cmyk_t>>::view_t planar_cmyk_view(
    std::size_t width,
    std::size_t height,
    IC c,
    IC m,
    IC y,
    IC k,
    std::ptrdiff_t rowsize_in_bytes)
{
    using view_t = typename type_from_x_iterator<planar_pixel_iterator<IC, cmyk_t>>::view_t;
    return view_t(
        width,
        height,
        typename view_t::locator(planar_pixel_iterator<IC, cmyk_t>(c, m, y, k), rowsize_in_bytes));
}

but if the is changed to trailing return type, then the reformatting output is nicer, I think:

template <typename IC>
inline auto planar_cmyk_view(
    std::size_t width,
    std::size_t height,
    IC c,
    IC m,
    IC y,
    IC k,
    std::ptrdiff_t rowsize_in_bytes) ->
    typename type_from_x_iterator<planar_pixel_iterator<IC, cmyk_t>>::view_t
{
    using view_t = typename type_from_x_iterator<planar_pixel_iterator<IC, cmyk_t>>::view_t;
    return view_t(
        width,
        height,
        typename view_t::locator(planar_pixel_iterator<IC, cmyk_t>(c, m, y, k), rowsize_in_bytes));
}

[mloskot]

I'm happy to go for this - it is good enough and better is evil of good, especially that we have not been able to work out any better for quite some time due to clang-format limitations :-)

We still need to consider how to approach the "big reformat" - see the MongoDB story I linked in #518 (comment)

[sdebionne]

It looks like clang-tools 12 is required? With 11.1, I get:

Invalid argument
YAML:6:25: error: invalid boolean
AlignConsecutiveMacros: AcrossEmptyLinesAndComments
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~

[lpranam]

@sdebionne yes clang 12 is required.

[mloskot]

I understand clang 12 is not yet widely available via popular distro packages, but I think we should start using clang-format from the latest release, and keep ourselves updated as soon as new version is released.

[mloskot]

I have another example and question:

I run the clang-format against header from the PR #604 and wonder if this alignment of = assignments is intentional?

     for (std::ptrdiff_t i = 0; i < 16; i++)
     {
-        pointers[i] = src_loc.cache_location(points[i][0], points[i][1]);
+        pointers[i]        = src_loc.cache_location(points[i][0], points[i][1]);
         intensity_array[i] = src_loc[pointers[i]];
     }

I'm not convinced as it may artificially lengthen lines and cause unnecessary line wrapping.
What you think @lpranam @sdebionne ?

---

Here is similar situation where all the score > fast_... lines got aligned to the right

if (!nonmax || score > fast_score_matrix(j - 1, i)[0] &&
                    score > fast_score_matrix(j + 1, i)[0] &&
                    score > fast_score_matrix(j - 1, i - 1)[0] &&
                    score > fast_score_matrix(j, i - 1)[0] &&
                    score > fast_score_matrix(j + 1, i - 1)[0] &&
                    score > fast_score_matrix(j - 1, i + 1)[0] &&
                    score > fast_score_matrix(j, i + 1)[0] &&
                    score > fast_score_matrix(j + 1, i + 1)[0])
{
    keypoints.push_back(u);
    scores.push_back(score);
}

I, personally, do not like it as the big blank space to the left is just wasteful - I don't you know but I read from left to right :-)

[lpranam]

yes it was intentional, but we can change I don't mind anything in this case

[sdebionne]

Not big fan of the alignment of = assignments neither. As of the alignment to the right, I have never seen anything like that before. I am all for a consistent formatting, really don't care too much about the rules as long as they are not too artificial, not too far from what a human developer (think lazy) would write 😁

[mloskot]

Good. I don't think I'd ever hit the TAB so many times myself :-)

So, I suggest to avoid those alignments.

[mloskot]

Replying to my own #596 (comment)

Do you know if there is any way in clang-format 12 to handle the trailing return better way?

There is a way! https://stackoverflow.com/a/60471935/151641 which, I think, we should consider for complex prototypes. For example, without the hack :

template <typename String, typename FormatTag, typename ConversionPolicy>
inline auto make_reader(
    String const& file_name,
    FormatTag const&,
    ConversionPolicy const& cc,
    typename std::enable_if<
        mp11::mp_and<detail::is_supported_path_spec<String>, is_format_tag<FormatTag>>::value>::
        type* = nullptr) -> typename get_reader<String, FormatTag, ConversionPolicy>::type
{
    return make_reader(file_name, image_read_settings<FormatTag>(), cc);
}

and with the hack is better, I think

template <typename String, typename FormatTag, typename ConversionPolicy>
inline auto make_reader(
    String const& file_name,
    FormatTag const&,
    ConversionPolicy const& cc,
    typename std::enable_if<
        mp11::mp_and<detail::is_supported_path_spec<String>, is_format_tag<FormatTag>>::value>::
        type* = nullptr)  //
    -> typename get_reader<String, FormatTag, ConversionPolicy>::type
{
    return make_reader(file_name, image_read_settings<FormatTag>(), cc);
}

[mloskot]

I updated this PR with commit a3af622 with proposal of tweaks, please have a look

Here is `test.hpp` sample with output of my tweaks

//
// Copyright 2007-2012 Christian Henning, Andreas Pokorny, Lubomir Bourdev
//
// Distributed under the Boost Software License, Version 1.0
// See accompanying file LICENSE_1_0.txt or copy at
// http://www.boost.org/LICENSE_1_0.txt
//
#ifndef BOOST_GIL_IO_READ_VIEW_HPP
#define BOOST_GIL_IO_READ_VIEW_HPP

#include <boost/gil/detail/mp11.hpp>
#include <boost/gil/io/base.hpp>
#include <boost/gil/io/conversion_policies.hpp>
#include <boost/gil/io/device.hpp>
#include <boost/gil/io/get_reader.hpp>
#include <boost/gil/io/path_spec.hpp>

#include <boost/mp11.hpp>

#include <type_traits>

namespace boost { namespace gil {

/// \ingroup IO

/// \brief Reads an image view without conversion. No memory is allocated.
/// \param reader An image reader.
/// \param view The image view in which the data is read into.
/// \param settings  Specifies read settings depending on the image format.
/// \throw std::ios_base::failure
template <typename Reader, typename View>
inline void read_view(
    Reader reader,
    View const& view,
    typename std::enable_if<mp11::mp_and<
        detail::is_reader<Reader>,
        typename is_format_tag<typename Reader::format_tag_t>::type,
        typename is_read_supported<
            typename get_pixel_type<View>::type,
            typename Reader::format_tag_t>::type>::value>::type* = nullptr)
{
    reader.check_image_size(view.dimensions());
    reader.init_view(view, reader._settings);
    reader.apply(view);
}

template <typename String, typename FormatTag, typename ConversionPolicy>
inline auto make_reader(
    String const& file_name,
    FormatTag const&,
    ConversionPolicy const& cc,
    typename std::enable_if<
        mp11::mp_and<detail::is_supported_path_spec<String>, is_format_tag<FormatTag>>::value>::
        type* = nullptr)  //
    -> typename get_reader<String, FormatTag, ConversionPolicy>::type
{
    return make_reader(file_name, image_read_settings<FormatTag>(), cc);
}

template <typename FormatTag, typename ConversionPolicy>
inline auto make_reader(
    const std::wstring& file_name,
    const FormatTag&,
    const ConversionPolicy& cc)  //
    -> typename get_reader<std::wstring, FormatTag, ConversionPolicy>::type
{
    return make_reader(file_name, image_read_settings<FormatTag>(), cc);
}

void foo()
{
    for (std::ptrdiff_t i = 0; i < 16; i++)
    {
        pointers[i] = src_loc.cache_location(points[i][0], points[i][1]);
        pointers[i] = src_loc.cache_location(points[i][0], points[i][1]) + src_loc[pointers[i]]
                    - src_loc[pointers[i]];
        intensity_array[i] = src_loc[pointers[i]];
    }

    for (std::ptrdiff_t i = 0; i < 16; i++)
    {
        if (!nonmax
            || (score > fast_score_matrix(j - 1, i)[0] && score > fast_score_matrix(j + 1, i)[0]
                && score > fast_score_matrix(j - 1, i - 1)[0]
                && score > fast_score_matrix(j, i - 1)[0]
                && score > fast_score_matrix(j + 1, i - 1)[0]
                && score > fast_score_matrix(j - 1, i + 1)[0]
                && score > fast_score_matrix(j, i + 1)[0]
                && score > fast_score_matrix(j + 1, i + 1)[0]))
        {
            keypoints.push_back(u);
            scores.push_back(score);
        }
    }

    if (!nonmax
        || (score > fast_score_matrix(j - 1, i)[0] && score > fast_score_matrix(j + 1, i)[0]
            && score > fast_score_matrix(j - 1, i - 1)[0] && score > fast_score_matrix(j, i - 1)[0]
            && score > fast_score_matrix(j + 1, i - 1)[0]
            && score > fast_score_matrix(j - 1, i + 1)[0] && score > fast_score_matrix(j, i + 1)[0]
            && score > fast_score_matrix(j + 1, i + 1)[0]))
    {
        keypoints.push_back(u);
        scores.push_back(score);
    }
}

struct tencil_5points
{
    double delta_t = 0.25;

    template <typename SubImageView>
    auto compute_laplace(SubImageView view, point_t origin)
        -> stencil_type<typename SubImageView::value_type>
    {
        auto current = view(origin);
        stencil_type<typename SubImageView::value_type> stencil;
        using channel_type = typename channel_type<typename SubImageView::value_type>::type;
        std::array<gil::point_t, 8> offsets(get_directed_offsets());
        typename SubImageView::value_type zero_pixel;
        static_fill(zero_pixel, 0);
        for (std::size_t index = 0; index < offsets.size(); ++index)
        {
            if (index % 2 != 0)
            {
                static_transform(
                    view(origin.x + offsets[index].x, origin.y + offsets[index].y),
                    current,
                    stencil[index],
                    std::minus<channel_type>{});
            }
            else
            {
                stencil[index] = zero_pixel;
            }
        }
        return stencil;
    }

    template <typename Pixel>
    Pixel reduce(const stencil_type<Pixel>& stencil)
    {
        auto first = stencil.begin();
        auto last = stencil.end();
        using channel_type = typename channel_type<Pixel>::type;
        auto result = []() {
            Pixel zero_pixel;
            static_fill(zero_pixel, channel_type(0));
            return zero_pixel;
        }();

        for (std::size_t index : {1u, 3u, 5u, 7u})
        {
            static_transform(result, stencil[index], result, std::plus<channel_type>{});
        }
        Pixel delta_t_pixel;
        static_fill(delta_t_pixel, delta_t);
        static_transform(result, delta_t_pixel, result, std::multiplies<channel_type>{});

        return result;
    }
};

void foo()
{
}

}}  // namespace boost::gil

#endif

I think we are close to merge this, once for all and for good :)

[sdebionne]

In case anyone is interested, I have just pushed a re-formated develop here. So far I am happy with the new formatting.

I noticed the io extension uses:

-    template< typename View_Dst
-            , typename View_Src
-            >
-    void copy_data( const View_Dst&              dst
-                  , const View_Src&              src
-                  , typename View_Dst::y_coord_t y
-                  , std::true_type // is gray1_view
-                  )
+    template <typename View_Dst, typename View_Src>
+    void copy_data(
+        const View_Dst& dst,
+        const View_Src& src,
+        typename View_Dst::y_coord_t y,
+        std::true_type  // is gray1_view

I kinda like the comma in front and the closing > and ) on a new line. What do you think?

I'll try to rewrite the complete history and push it in a new fork.

[sdebionne]

One more comment: one-liner member functions are now expended:

-    static void apply(V const &, Value const &) { throw std::bad_cast();}
+    static void apply(V const&, Value const&)
+    {
+        throw std::bad_cast();
+    }

Is that configurable?

[mloskot]

@sdebionne Awesome! I'll check it in details later tonight.

I kinda like the comma in front and the closing > and ) on a new line. What do you think?

It's been my long time preference, especially for members initialisation in constructors e.g. in Boost.Geometry

One more comment: one-liner member functions are now expended:
Is that configurable?

Yes, AllowShortFunctionsOnASingleLine should do it.

[lpranam]

@sdebionne why try to rewrite the history? boost does not allow force push so ultimately things become not usable in this case? or am I misinterpreting something?

[mloskot]

The branch protection that forbids forced-push is ours in boostorg/gil settings.

I recall I was warning you @lpranam about not doing the force-push though :-)

However, we, @stefanseefeld and myself, recreated develop branch from master branch and we had to 1) disable the branch protection 2) force-push 3) re-enable the branch protection as it was effectively rewrite of history of develop.
It was discussed in our Gitter, but it takes some serching - I wish it was discussed on boost-gil ML :-)
For example, here is a trace of the discussion.

The superproject at boostorg/boost automagically updates the references every half an hour or so, AFAIR.

[lpranam]

in that case, I have done such thing in past on one of my project let me try to find if I can get my hands on the command/script

[sdebionne]

Here is a gil-reformatted repo. I used lint_history:

lint-history --relevant 'return filename.endswith(b".cpp") or filename.endswith(b".hpp")' clang-format -style=file -i

Pretty fast, so not a pb if it needs to be run again with the modifications discussed above.

At some points we will need to unprotect the protected branches, force push the new history, protect again. It's best to do that when we have the least WIP PRs, since every contributors will need to rebase on the new history. A bit of git gymnastic...

[sdebionne]

AppVeyor fails with fatal: unable to access 'https://github.com/boostorg/boost.git/': Could not resolve host: github.com. Do we still need this service, or is it fully covered by Azure / GA?

[lpranam]

GA is not covering stdcxx = 11 on msvc but definitely we can do something about it

[mloskot]

@lpranam

GA is not covering stdcxx = 11 on msvc but definitely we can do something about it

There is no such thing as plain C++11 for MSVC, only C11. VS 2015, 2017, 2019 default to C++14.

Following @sdebionne suggestion, I'm going to get rid of AppVeyor - less is more :-) - #618

[sdebionne]

Do we need to pin the version of clang-format or is it backward compatible? e.g. given a .clang-format config will I get the same formatting with clang-format 10 and clang-format 13?

[sdebionne]

Bummer, I have just checked with my current project, switching from v10 to v12: formatting is a little different, some whitespaces added in a few files (3 out of hundreds)...

[mloskot]

switching from v10 to v12: formatting is a little different,

Yes, that is known PITA of clang-format, from MongoDB experience:

You have to ensure that everyone uses the same version of ClangFormat, or eventually you will run into config incompatibilities, and maybe even silent formatting discrepancies.

I don't mind starting with the current latest. Hopefully, future releases of clang-format are going to receive less and less incompatibilities.

[mloskot]

@sdebionne

At some points we will need to unprotect the protected branches, force push the new history, protect again. It's best to do that when we have the least WIP PRs, since every contributors will need to rebase on the new history.

I think, the only still open PRs that are going to be merged are the GSoC ones and some several of yours and @lpranam 's
Most of others are less important examples, GSoC tests, or work that needs some thorough refactoring (e.g. merge multiple PRs addressing the same algorithm).

We may want to take the Boost release calendar into account: Summer release on the second Wednesday of August. IOW, either we do it soon or after the next release.

[mloskot]

@sdebionne I'm looking at the develop branch in the https://github.com/sdebionne/gil-reformated and comparing the formatting with the current one to see the overall formatting effect.

Did you use @lpranam 's .clang-format from this PR?

https://github.com/sdebionne/gil-reformated/blob/d85dff7b77f8eacd8713a4692ffeeb43c89b3009/include/boost/gil/algorithm.hpp#L33-L37 displays the unbroken templates:

template <typename ChannelPtr, typename ColorSpace>
struct planar_pixel_iterator;
template <typename Iterator> class memory_based_step_iterator;
template <typename StepIterator> class memory_based_2d_locator;

while @lpranam 's configuration contains

gil/.clang-format

Line 26 in a3af622

AlwaysBreakTemplateDeclarations: Yes

[sdebionne]

Did you use @lpranam 's .clang-format from this PR?

This was my intention, let me check...

[sdebionne]

Sorry for the delay, I accidentally overwrote my .clang-format, so was not able to tell if it was actually the result of git checkout lpranam:format -- .clang-format or if it's an issue with clang-format that needs to be run multiple time to get the correct result.

So I have reran the all thing from scratch, using the latest .clang-format, and I get the same result than the one published on my gil-reformat fork. Rerunning lint-history, I get the correct AlwaysBreakTemplateDeclarations = Yes formatting. Conclusion: lint-history needs to be run multiple time until clang-format converges, which is similar to what we observed with our project.

[mloskot]

@sdebionne Thanks for dobule checking, it makes sense to me.

Folks, what's your preference about #596 (comment) about scheduling the big reformat? /cc @lpranam @simmplecoder

[lpranam]

i'd suggest first week after the release.

[lpranam]

So boost 1.77 is released now we can move ahead with refactoring now...

[mloskot]

I say YES!
I am swamped with work & house construction, but I'm hoping a free weekend is coming soon, so I can finally catch up with all the GIL backlog.