kdev-astyle : upgrade libastyle to v3.1
ClosedPublic

Authored by rjvbb on Sep 19 2018, 6:06 PM.

Details

Summary

This is a split-off of D15532; it updates the embedded libastyle copy to upstream v3.1

Test Plan

works as intended AFAICT.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb requested review of this revision.Sep 19 2018, 6:06 PM
rjvbb created this revision.
apol added a subscriber: apol.Sep 19 2018, 6:36 PM

Is there a reason why it can't be a proper dependency?

pino added a subscriber: pino.Sep 19 2018, 6:38 PM

What about using the system version of libastyle, instead? I.e. look for libastyle, and if not found then do not build kdev-astyle.
This would avoid a code copy which clearly is not kept up-to-date, with almost no overhead on packagers (since astyle provides a shared library for a long time).
Since astyle 3.0 broke the ABI and changed its API a bit, using that version as minimum could be a reasonable idea.

rjvbb added a comment.Sep 19 2018, 8:34 PM

Normally I'd agree, but here we're dealing with really a tiny amount of additional code and a project which isn't exactly being kept up-to-date by distributions either (on Linux I'd get 2.04 from the system, on Mac 2.05 from MacPorts). Bundling the few files involved means that at least everyone gets the astyle plugin and with the same version.

pino added a comment.Sep 19 2018, 9:01 PM

Normally I'd agree, but here we're dealing with really a tiny amount of additional code and a project which isn't exactly being kept up-to-date by distributions either (on Linux I'd get 2.04 from the system, on Mac 2.05 from MacPorts).

Not really: https://repology.org/metapackage/astyle/versions
So indeed MacPorts seems not up to the other distributions, not even homebrew. Still, not a reason to keep bundling astyle, just for an old version in MacPorts.

Not really: https://repology.org/metapackage/astyle/versions

That table ought to be sorted following usage data from distrowatch; did you see the versions in Debian for instance?

I really think that in this case it is hardly worth the hassle of checking the available version. Has anyone even noticed issues with formatting? I haven't, and I wouldn't have bothered upgrading if it weren't for those few fixed bugs in ObjC formatting.

kfunk accepted this revision.Sep 20 2018, 6:42 AM
kfunk added a subscriber: kfunk.

Go for it, please push to 5.3 branch.

I agree with Pino et al. that we should try to use distro packages where available, but I'd wait for Debian Stable to at least have libastyle >=v3.0 around. We can probably start adding libastyle detection to our CMake code, and keep falling back on using our internal copy if unavailable, but that'd be another patch.

This revision is now accepted and ready to land.Sep 20 2018, 6:42 AM
This revision was automatically updated to reflect the committed changes.

With asserts enabled, I get a crash with this on opening the settings or sometimes only when selecting Objective-C in the formatting settings

kdevelop: /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp:3841: std::__cxx11::string astyle::ASFormatter::peekNextText(const string&, bool, std::shared_ptr<astyle::ASPeekStream>) const: Assertion `sourceIterator->getPeekStart() == 0 || streamArg != nullptr' failed.

Seems that sourceIterator->getPeekStart() == 0 is not fulfilled by the new implementation done for KDevelop's AStyleStringIterator::getPeekStart(). This needs some investigation, and more testing please.

This is the backtrace:

#6  0x00007fb33b1ae08b in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#7  0x00007fb33b1974e9 in __GI_abort () at abort.c:79
#8  0x00007fb33b1973c1 in __assert_fail_base (fmt=0x7fb33b2fb0f0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x7fb2f957ba68 "sourceIterator->getPeekStart() == 0 || streamArg != nullptr", file=0x7fb2f957b6b8 "/home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp", line=3841, function=<optimized out>) at assert.c:92
#9  0x00007fb33b1a66f2 in __GI___assert_fail (assertion=0x7fb2f957ba68 "sourceIterator->getPeekStart() == 0 || streamArg != nullptr", file=0x7fb2f957b6b8 "/home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp", line=3841, function=0x7fb2f957c800 <astyle::ASFormatter::peekNextText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::shared_ptr<astyle::ASPeekStream>) const::__PRETTY_FUNCTION__> "std::__cxx11::string astyle::ASFormatter::peekNextText(const string&, bool, std::shared_ptr<astyle::ASPeekStream>) const") at assert.c:101
#10 0x00007fb2f9558eae in astyle::ASFormatter::peekNextText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::shared_ptr<astyle::ASPeekStream>) const (this=0x4c4cdb0, firstLine="a,*d--);", endOnEmptyLine=false, streamArg=std::shared_ptr<class astyle::ASPeekStream> (empty) = {...}) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp:3841
#11 0x00007fb2f9556a23 in astyle::ASFormatter::isPointerOrReference() const (this=0x4c4cdb0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp:3177
#12 0x00007fb2f9553692 in astyle::ASFormatter::nextLine[abi:cxx11]() (this=0x4c4cdb0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp:1775
#13 0x00007fb2f952fa52 in AStyleFormatter::formatSource(QString const&, QString const&, QString const&) (this=0x4c4cdb0, text=..., leftContext=..., rightContext=...) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/astyle_formatter.cpp:49
#14 0x00007fb2f9521c0a in AStylePlugin::formatSourceWithStyle(KDevelop::SourceFormatterStyle, QString const&, QUrl const&, QMimeType const&, QString const&, QString const&) const (this=0x4369850, s=..., text=..., mime=..., leftContext=..., rightContext=...) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/astyle_plugin.cpp:266
#15 0x00007fb33fb0eab6 in KDevelop::SourceFormatterSelectionEdit::updatePreview() (this=0x5b08e10) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/sourceformatterselectionedit.cpp:564
#16 0x00007fb33fb0bccb in KDevelop::SourceFormatterSelectionEdit::resetUi() (this=0x5b08e10) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/sourceformatterselectionedit.cpp:289
#17 0x00007fb33fb0b699 in KDevelop::SourceFormatterSelectionEdit::loadSettings(KConfigGroup const&) (this=0x5b08e10, config=...) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/sourceformatterselectionedit.cpp:243
#18 0x00007fb33fb39497 in SourceFormatterSettings::reset() (this=0x5b494f0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/settings/sourceformattersettings.cpp:58
#19 0x00007fb33fb0477c in KDevelop::ConfigDialog::addConfigPageInternal(KPageWidgetItem*, KDevelop::ConfigPage*) (this=0x7ffd91d692a0, item=0x5e49ed0, page=0x5b494f0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/configdialog.cpp:170
#20 0x00007fb33fb04510 in KDevelop::ConfigDialog::appendConfigPage(KDevelop::ConfigPage*) (this=0x7ffd91d692a0, page=0x5b494f0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/configdialog.cpp:147
#21 0x00007fb33fa5e9ff in KDevelop::UiController::showSettingsDialog() (this=0x2372b30) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/uicontroller.cpp:512
#22 0x00007fb33fa40c45 in KDevelop::MainWindowPrivate::settingsDialog() (this=0x24314a0) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/shell/mainwindow_actions.cpp:112
plugins/astyle/astyle_stringiterator.cpp
77

Is there some API dox what one should implement for getPeekStart(), especially which values are valid?

rjvbb added a comment.EditedSep 21 2018, 8:58 AM

Is there some API dox what one should implement for getPeekStart(), especially which values are valid?

Haven't looked for any, I just copied the example I found because the relevant member variable was there, and then didn't look any further because the code works fine for me. Curious (and how I hate this kind of assert...)

Edit: getPeekStart() is only called in that assert, no wonder I didn't get a crash. If I don't misread the check the function is supposed to return 0 unless streamArg!=nullptr, which *probably* means that our m_peekStart` should be initialised to 0, not -1

rjvbb reopened this revision.Sep 21 2018, 9:50 AM

Would this be OK as a fix?

diff --git a/plugins/astyle/astyle_stringiterator.cpp b/plugins/astyle/astyle_stringiterator.cpp
index 27326cf7db502b40624ca915ae61212b9b1a97a2..da802b029018156390ebefab57ab0010423aac80 100644
--- a/plugins/astyle/astyle_stringiterator.cpp
+++ b/plugins/astyle/astyle_stringiterator.cpp
@@ -74,7 +74,7 @@ void AStyleStringIterator::peekReset()
 
 astyle::streamoff AStyleStringIterator::getPeekStart() const
 {
-   return m_peekStart;
+   return m_peekStart == -1 ? 0 : m_peekStart;
 }

A slightly more elaborate version would use a separate bool to track whether m_peekStart is valid or not, let me know if that's justified.

This revision is now accepted and ready to land.Sep 21 2018, 9:50 AM

Would this be OK as a fix?

I cannot tell, as I still haven't found any API docs for astyle, at least one which has a comment about this new method. So I have no idea what contract one should implement for ASSourceIterator::getPeekStart() and what values are expected when. In the code we have imported here there is only a single usage of that call, which is in that assert.
"If you have inherited this class to access the formatter, you will need to add a method similar to getPeekStart() in the ASStreamIterator class in astyle_main.h." from http://astyle.sourceforge.net/news.html is the only info I have, which leaves lots of room.
Have you seen any docs for that method?

Based on that a fix could also be to always return 0 ;)

Your proposed fix might work or not, from what I understood on the quick look at astyle code that assert wants to make sure any other peeking has been done, so peekStart == 0 means kind of the no peeking on-going.
In any case that method implementation needs to have some comment that we added ti without really knowing what it should do, so future code readers do not have to scratch their head what we knew they do not.

"If you have inherited this class to access the formatter, you will need to add a method similar to getPeekStart() in the ASStreamIterator class in astyle_main.h." from http://astyle.sourceforge.net/news.html is the only info I have, which leaves lots of room.
Have you seen any docs for that method?

No, but AStreamIterator::getPeekStart() just returns AStreamIterator::peekStart (hence my version returning m_peekStart).
Looking at astyle_main.cpp we see that peekStart initialised to 0, and elsewhere updated as

	if (!peekStart)
		peekStart = inStream->tellg();

IOW, their code uses 0 as the invalid value, with the border case inStream->tellg()==0 (which presumable happens only once *if* peekStart is ever changed at the beginning).

Based on that a fix could also be to always return `0` ;)

Which would be fine as long as we use our own copy (in which case we could also get rid of the assert because it is rather pointless), or even of the entire missing method.

Your proposed fix might work or not, from what I understood on the quick look at astyle code

I can confirm that it does prevent the assert from triggering.

You're right that a comment would be in order.

kfunk added a comment.Sep 25 2018, 9:12 AM

Note: Reported as https://bugs.kde.org/show_bug.cgi?id=399048 -- @Rene: Please push your fix(?) -- looks legit.

Surprisingly, getPeekStart() isn't even used besides that one call in assert() in Astyle sources(?)...

This revision was automatically updated to reflect the committed changes.