search

scanse / sweep-sdk

Sweep SDK
MIT License
90 stars 85 forks source link

fix_cpp_interface #107

Closed MikeGitb closed 7 years ago

MikeGitb commented 7 years ago

Scope of changes

Member functions of sweep::sweep are implemented in header but where not marked as inline, which leads to linker errors as soon as the header is included in separate TUs.

This PR fixes this and refactors the get_scan function a bit.

dcyoung commented 7 years ago

which leads to linker errors as soon as the header is included in separate TUs

Yikes. All of our examples thus far have been single files. Glad you caught this.

I gave the branch a quick build and test on both linux and windows without issue. But that was just using the included examples. I'm assuming you've tested with a project with multiple translation units?

Could you format the file using the included libsweep/.clang-format file. If not, I can do so before any merge. Otherwise I don't see any issues.

@daniel-j-h Any reason not to merge?

MikeGitb commented 7 years ago

I have to admit I did not test the real code (beyond it compiling at all) but I made the same modifications to my private fork, which is almost identical and there it worked.

daniel-j-h commented 7 years ago

@daniel-j-h Any reason not to merge?

Looks good to me. 👍 for clang-format before merging. @MikeGitb thanks for fixing this!

MikeGitb commented 7 years ago

What specifically is bothering you? Just the long lines or also the colum alignment in the loop body?

I deliberately decided to format the latter this way, because it imho increases readability and if it isn't bothering you I'd guard those lines with // clang-format off / // clang-format on to keep it that way. Otherwise I can of course just clang-format over everything