Review Request 124427: kdevplatform: Make ctors explicit to avoid implicit construction (krazy2)

Kevin Funk kfunk at kde.org
Fri Jul 24 10:30:26 UTC 2015 


> On July 24, 2015, 9:16 a.m., Milian Wolff wrote:
> > LGTM, but what did you mean by "these changes break code with std::move" - can you please clarifiy?
> 
> Boris Egorov wrote:
>     Tests fail to pass. I removed failing lines, you can see them in diff. They are working with 'moved-from' objects. For example, test them for emptiness.
> 
> Milian Wolff wrote:
>     OK, I just looked at the diff in more depth (it was too long, sorry for my tl;dr; fail!)
>     
>     How did the change to add explicit ctors fail these tests? Do you know? I would assume it should be unrelated? Also, some parts of these tests I added deliberately afaik. And testing for emptiness of an moved-from object sounds perfectly fine to me. Why did you remove that?
> 
> Boris Egorov wrote:
>     I don't know how exactly my changes broke tests. But I ran them before and after changes, and they definitely started failing after.
>     
>     Like I said in commit message, I read in some[1] resources[2] that move-from object remains in valid, but not unspecified state. So we can delete them or assign to them, but we should not do any assumptions about their state or contents. Moved strings can be not empty, for example (I saw exactly this behaviour in failed tests).
>     
>     I can be wrong, I haven't worked with std::move much.
>     
>         1: http://en.cppreference.com/w/cpp/utility/move
>         2: http://stackoverflow.com/questions/20850196/what-lasts-after-using-stdmove-c11/20850223#20850223
> 
> Boris Egorov wrote:
>     "move-from object remains in valid, but not unspecified state" => "moved-from objects remain in valid, but unspecified state"
> 
> Milian Wolff wrote:
>     We implement the move semantics, well - I did. And I wanted to explictily keep them in a somewhat sane state afterwards. Moving from it means an item becomes invalid/empty. Thus calling `QVERIFY(movedFrom.isEmpty())` should still work. If not, then there is an issue which we _must_ debug. I would actually say please revert that part of this patch, and also the corresponding "explicit" ctors that trigger the tests failures. Then we can discuss on the ML together why this happens. In general, disabling tests in such a way is seldom a good idea, _especially_ if you are not sure why they suddenly break.
> 
> Boris Egorov wrote:
>     Oh, I didn't know that and assumed it was some kind of redundant test. I'm sorry. We are lucky you spotted these changes.
>     
>     I haven't detected the exact ctors triggering failures, but I'll try to find them and revert them today. Most likely candidates are Identifier and IndexedString whose tests fail.

@Milian: Sorry, I didn't notice the hunks in those .cpp files either. For some reason Reviewboard was showing only parts of the patch.

Indeed, disabling/removing those lines from the tests is a bad idea.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124427/#review82861
-----------------------------------------------------------


On July 23, 2015, 9:38 a.m., Boris Egorov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124427/
> -----------------------------------------------------------
> 
> (Updated July 23, 2015, 9:38 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Some constructors cannot be made explicit without further code
> modifications (project fails to build). Examples:
> 
>     IndexedDeclaration
>     IndexedQualifiedIdentifier
>     ReferencedTopDUContext
>     IndexedDUContext
>     IndexedTopDUContext
>     LocalIndexedDeclaration
> 
> Somehow these changes break tests with std::move. Some sources said
> that "moved-from" variables is in unspecified state after moving, so
> let's disable assertions working with such variables.
> 
> 
> Diffs
> -----
> 
>   debugger/breakpoint/breakpointdetails.h dff8cf2 
>   debugger/breakpoint/breakpointmodel.h c12e8fa 
>   debugger/framestack/framestackmodel.h fa1cbab 
>   debugger/framestack/framestackwidget.h 9ee5a83 
>   debugger/interfaces/ibreakpointcontroller.h c39f497 
>   debugger/interfaces/iframestackmodel.h 01b2545 
>   debugger/interfaces/ivariablecontroller.h d501a04 
>   debugger/util/pathmappings.h ffe58f2 
>   debugger/util/treemodel.h 71704c0 
>   debugger/variable/variablecollection.h af732c3 
>   debugger/variable/variablewidget.h 6a47497 
>   documentation/documentationview.h 6c8cd50 
>   interfaces/context.h 92f0448 
>   interfaces/iassistant.h 61b6046 
>   interfaces/idebugcontroller.h 72e52a4 
>   interfaces/idocumentcontroller.h 3036cae 
>   interfaces/ilanguagecontroller.h 04eb259 
>   interfaces/ipartcontroller.h 4fed10e 
>   interfaces/iplugincontroller.h b77a8aa 
>   interfaces/iproject.h f0f38d6 
>   interfaces/iprojectcontroller.h 86c2dfd 
>   interfaces/iprojectprovider.h 1e76dbd 
>   interfaces/iruncontroller.h e342f6c 
>   interfaces/iselectioncontroller.h fb65377 
>   interfaces/isession.h 1c1ee0a 
>   interfaces/isourceformatter.h f35748f 
>   interfaces/isourceformattercontroller.h efe03c1 
>   interfaces/launchconfigurationpage.h 2589325 
>   language/assistant/renameassistant.h cfd3d67 
>   language/assistant/staticassistant.h 4ca18c3 
>   language/assistant/staticassistantsmanager.h ab5430e 
>   language/backgroundparser/backgroundparser.h 4a79651 
>   language/backgroundparser/documentchangetracker.h bacd0b5 
>   language/backgroundparser/parseprojectjob.h 82e8b02 
>   language/backgroundparser/urlparselock.h 6005f9f 
>   language/codecompletion/codecompletionmodel.h b22cd97 
>   language/codecompletion/codecompletionworker.h e2fb39b 
>   language/codecompletion/normaldeclarationcompletionitem.h c19f4b2 
>   language/codegen/applychangeswidget.h 12fe48d 
>   language/codegen/archivetemplateloader.h 886c7ec 
>   language/codegen/astchangeset.h 000e5ae 
>   language/codegen/basicrefactoring.h 72ae54a 
>   language/codegen/duchainchangeset.h 0465da3 
>   language/codegen/sourcefiletemplate.h 4810443 
>   language/codegen/utilities.h d8e00e6 
>   language/duchain/aliasdeclaration.h 251c2e5 
>   language/duchain/builders/dynamiclanguageexpressionvisitor.h f06607a 
>   language/duchain/classdeclaration.h 128cf05 
>   language/duchain/classfunctiondeclaration.h fe32111 
>   language/duchain/classmemberdeclaration.h a9c9487 
>   language/duchain/declaration.h c5f2450 
>   language/duchain/declarationid.h dd3c050 
>   language/duchain/duchainbase.h 3c33e71 
>   language/duchain/duchaindumper.h 87e91b0 
>   language/duchain/duchainlock.h 0a16df1 
>   language/duchain/duchainpointer.h 3fd906f 
>   language/duchain/ducontextdynamicdata.h c0a8472 
>   language/duchain/forwarddeclaration.h 3deae92 
>   language/duchain/functiondeclaration.h e206c54 
>   language/duchain/functiondefinition.h 237bddb 
>   language/duchain/identifier.h 37fef57 
>   language/duchain/localindexedducontext.h a8eae2a 
>   language/duchain/namespacealiasdeclaration.h 817631a 
>   language/duchain/navigation/abstractnavigationcontext.h 29d5784 
>   language/duchain/navigation/problemnavigationcontext.h f19aa18 
>   language/duchain/navigation/usescollector.h b18f769 
>   language/duchain/navigation/usesnavigationcontext.h 61ff1fe 
>   language/duchain/navigation/useswidget.h 94087d7 
>   language/duchain/parsingenvironment.h f5524ce 
>   language/duchain/problem.h b8454fb 
>   language/duchain/tests/test_identifier.cpp 1799946 
>   language/duchain/topducontextdata.h b9de0df 
>   language/duchain/topducontextdynamicdata.h 1bdc465 
>   language/duchain/types/abstracttype.h 7613168 
>   language/duchain/types/arraytype.h 7a50ec6 
>   language/duchain/types/constantintegraltype.h 445af1f 
>   language/duchain/types/containertypes.h b69e335 
>   language/duchain/types/delayedtype.h 3954009 
>   language/duchain/types/enumerationtype.h 61b8cfa 
>   language/duchain/types/enumeratortype.h 3ba420f 
>   language/duchain/types/functiontype.h f1b15ea 
>   language/duchain/types/integraltype.h a93820e 
>   language/duchain/types/pointertype.h 4f336cb 
>   language/duchain/types/referencetype.h 38c91ad 
>   language/duchain/types/structuretype.h ccc533f 
>   language/duchain/types/typealiastype.h 7eed573 
>   language/duchain/types/unsuretype.h 5d504b4 
>   language/duchain/use.h 5eff1f4 
>   language/editor/modificationrevision.h 1fdff87 
>   language/editor/modificationrevisionset.h 4093780 
>   language/highlighting/codehighlighting.h 76f7f01 
>   language/highlighting/configurablecolors.h 4a65172 
>   language/interfaces/codecontext.h 72b5a4a 
>   language/util/basicsetrepository.h 19faedb 
>   language/util/kdevhash.h 4ac8c0d 
>   outputview/outputdelegate.h adbc275 
>   outputview/outputmodel.h 8ec09e7 
>   plugins/appwizard/appwizardpagewidget.h 1d6e105 
>   plugins/appwizard/appwizardplugin.h a59c187 
>   plugins/appwizard/projecttemplatesmodel.h e9837fc 
>   plugins/appwizard/projectvcspage.h f587c86 
>   plugins/classbrowser/allclassesfolder.h 94b16da 
>   plugins/classbrowser/classbrowserplugin.h 0bcb8ff 
>   plugins/classbrowser/classmodelnode.h b1e7308 
>   plugins/classbrowser/projectfolder.h b721ee1 
>   plugins/codeutils/codeutilsplugin.h ca12e2a 
>   plugins/contextbrowser/browsemanager.h 51166cd 
>   plugins/contextbrowser/contextbrowser.h 09066cd 
>   plugins/cvs/commitdialog.h cba07c0 
>   plugins/cvs/cvsannotatejob.h ed6a3d9 
>   plugins/cvs/cvsdiffjob.h 7e9feae 
>   plugins/cvs/cvsjob.h 474381e 
>   plugins/cvs/cvslogjob.h ad32982 
>   plugins/cvs/cvsproxy.h 4cab04c 
>   plugins/cvs/cvsstatusjob.h 4eafff5 
>   plugins/cvs/importmetadatawidget.h 9c776f1 
>   plugins/documentswitcher/documentswitcherplugin.h e85a54d 
>   plugins/documentswitcher/documentswitchertreeview.h cba65c8 
>   plugins/documentview/kdevdocumentmodel.h d851f5a 
>   plugins/documentview/kdevdocumentselection.h 832484c 
>   plugins/execute/executeplugin.h 3e379b1 
>   plugins/execute/nativeappconfig.h 4eacf1e 
>   plugins/execute/projecttargetscombobox.h b29a7df 
>   plugins/executescript/executescriptplugin.h f4b2e60 
>   plugins/executescript/scriptappconfig.h 66dd716 
>   plugins/externalscript/editexternalscript.h 2b26ca7 
>   plugins/externalscript/externalscriptplugin.h 706c55b 
>   plugins/externalscript/externalscriptview.h f54f062 
>   plugins/filemanager/kdevfilemanagerplugin.h e543106 
>   plugins/filetemplates/classidentifierpage.h 8ce3da8 
>   plugins/filetemplates/licensepage.h 0134afb 
>   plugins/filetemplates/outputpage.h f01d392 
>   plugins/filetemplates/overridespage.h db4863a 
>   plugins/filetemplates/templateclassassistant.h 76e60f1 
>   plugins/filetemplates/templatepreview.h 0324fb5 
>   plugins/git/gitjob.h 8d0a8f2 
>   plugins/git/gitplugin.h 6bafc59 
>   plugins/grepview/grepjob.h cb4e81f 
>   plugins/grepview/grepoutputview.h 95c83dc 
>   plugins/grepview/grepviewplugin.h 68dfdb2 
>   plugins/outlineview/outlinenode.h 0e8d3f5 
>   plugins/patchreview/patchreview.h d7127b0 
>   plugins/problemreporter/problemhighlighter.h 8d566ab 
>   plugins/problemreporter/problemnavigationcontext.h e761d14 
>   plugins/problemreporter/problemreporterplugin.h 1014429 
>   plugins/problemreporter/problemsview.h ced44f2 
>   plugins/projectmanagerview/projectbuildsetwidget.h e843d3f 
>   plugins/projectmanagerview/projectmanagerviewplugin.h b180d25 
>   plugins/projectmanagerview/projecttreeview.h 6512157 
>   plugins/quickopen/duchainitemquickopen.h af28afe 
>   plugins/quickopen/expandingtree/expandingtree.h 17daa39 
>   plugins/quickopen/expandingtree/expandingwidgetmodel.h 6d9618e 
>   plugins/quickopen/projectfilequickopen.h 06c483d 
>   plugins/quickopen/projectitemquickopen.h 5651375 
>   plugins/quickopen/quickopenmodel.h 32e3e98 
>   plugins/quickopen/quickopenplugin.h 8449f07 
>   plugins/standardoutputview/toolviewdata.h a082f9a 
>   plugins/subversion/svnaddjob.h 03a1902 
>   plugins/subversion/svnaddjob_p.h 456ed96 
>   plugins/subversion/svnblamejob.h 562e0e8 
>   plugins/subversion/svnblamejob_p.h ebb77ff 
>   plugins/subversion/svncatjob.h 6fab39b 
>   plugins/subversion/svncatjob_p.h ee488fe 
>   plugins/subversion/svncheckoutjob.h a5b8f02 
>   plugins/subversion/svncheckoutjob_p.h 836e641 
>   plugins/subversion/svncheckoutmetadatawidget.h aec9cef 
>   plugins/subversion/svnclient.h 56fe837 
>   plugins/subversion/svncommitjob_p.h ae00109 
>   plugins/subversion/svncopyjob.h d146c77 
>   plugins/subversion/svncopyjob_p.h 8dd1d2c 
>   plugins/subversion/svndiffjob.h 2d9b099 
>   plugins/subversion/svndiffjob_p.h fed5385 
>   plugins/subversion/svnimportjob.h 1b4efee 
>   plugins/subversion/svnimportmetadatawidget.h 25d178b 
>   plugins/subversion/svninfojob.h 436bc3f 
>   plugins/subversion/svninfojob_p.h cb61e75 
>   plugins/subversion/svninternaljobbase.h 4f2940e 
>   plugins/subversion/svnjobbase.h 5f622cf 
>   plugins/subversion/svnlocationwidget.h 9c927ce 
>   plugins/subversion/svnlogjob.h dbd7c07 
>   plugins/subversion/svnlogjob_p.h c198c43 
>   plugins/subversion/svnmovejob.h b52c1ad 
>   plugins/subversion/svnmovejob_p.h 9055033 
>   plugins/subversion/svnremovejob.h d5dbc3d 
>   plugins/subversion/svnremovejob_p.h b0e8c41 
>   plugins/subversion/svnrevertjob.h 0c9d585 
>   plugins/subversion/svnrevertjob_p.h a385aa6 
>   plugins/subversion/svnssldialog.h 6ccc9b8 
>   plugins/subversion/svnstatusjob_p.h 2477ed1 
>   plugins/subversion/svnupdatejob.h 5e00b77 
>   plugins/subversion/svnupdatejob_p.h 31fcece 
>   plugins/switchtobuddy/switchtobuddyplugin.h a7b566c 
>   plugins/testview/testviewplugin.h 0fe8df1 
>   plugins/welcomepage/declarative/icoreobject.h 1244b56 
>   plugins/welcomepage/uihelper.h 78b1df6 
>   plugins/welcomepage/welcomepageview.h 1aee0e7 
>   project/projectbuildsetmodel.h 920ac1d 
>   project/projectchangesmodel.h 7fd84c9 
>   project/projectconfigskeleton.h 80e118b 
>   project/projectitemlineedit.h cda1807 
>   project/projectmodel.h 22ac4fa 
>   project/projectproxymodel.h 6feba14 
>   project/projectutils.h 03fce30 
>   serialization/indexedstring.h 7e06cab 
>   serialization/itemrepositoryregistry.h 7185555 
>   serialization/tests/test_indexedstring.cpp f0c7958 
>   shell/core_p.h 3caed06 
>   shell/debugcontroller.h a609e33 
>   shell/documentationcontroller.h 0be4b2e 
>   shell/documentcontroller.h 4c74860 
>   shell/editorconfigpage.h 593523b 
>   shell/filteredproblemstore.h 25c29d3 
>   shell/languagecontroller.h 28d1e7e 
>   shell/launchconfiguration.h 692bf5c 
>   shell/launchconfigurationdialog.h 799c844 
>   shell/loadedpluginsdialog.h 699475a 
>   shell/mainwindow_p.h fd23597 
>   shell/openprojectpage.h 5602b74 
>   shell/plugincontroller.h d4f48a6 
>   shell/problemmodel.h 48b7f1f 
>   shell/problemmodelset.h 5aa2836 
>   shell/problemstore.h c1426c0 
>   shell/problemstorenode.h 05844c2 
>   shell/project.h 49d69a4 
>   shell/projectcontroller.h 30db285 
>   shell/projectinfopage.h c58004b 
>   shell/projectsourcepage.h 80c2cfb 
>   shell/runcontroller.h 9e5ee2a 
>   shell/selectioncontroller.h ad77f85 
>   shell/session.h 2b5a09f 
>   shell/sessioncontroller.h db5c7d0 
>   shell/sessiondialog.h 8e97dfb 
>   shell/settings/sessionconfigskeleton.h 8c76493 
>   shell/sourceformattercontroller.h fcf181f 
>   shell/statusbar.h 295cc92 
>   shell/textdocument.h 64952e0 
>   shell/uicontroller.h 677ff3f 
>   shell/workingsets/workingset.h dd301c2 
>   shell/workingsets/workingsetwidget.h fdc1db9 
>   sublime/aggregatemodel.h c9d0caf 
>   sublime/container.h fecd865 
>   sublime/controller.h 61883d2 
>   sublime/holdupdates.h ca0ee37 
>   sublime/idealcontroller.h 8541191 
>   sublime/ideallayout.h 4baac33 
>   sublime/idealtoolbutton.h e8b2baf 
>   sublime/mainwindow_p.h 4a18f4c 
>   util/duchainify/main.h c0304ef 
>   util/environmentgrouplist.h 0412c5e 
>   util/executecompositejob.h 26d5617 
>   util/focusedtreeview.h f3bf065 
>   util/foregroundlock.h 3d994da 
>   util/objectlist.h c59b487 
>   util/processlinemaker.h 6a53922 
>   vcs/dvcs/dvcsjob.h 81c6f33 
>   vcs/dvcs/ui/dvcsimportmetadatawidget.h c037be2 
>   vcs/dvcs/ui/revhistory/commitView.h 046cd5f 
>   vcs/interfaces/icontentawareversioncontrol.h 2ef5882 
>   vcs/models/brancheslistmodel.h f96255d 
>   vcs/models/vcsfilechangesmodel.h e77aa1a 
>   vcs/models/vcsitemeventmodel.h 61966e4 
>   vcs/vcsjob.h 95af667 
>   vcs/vcslocation.h 3a270e8 
>   vcs/widgets/standardvcslocationwidget.h b1efd40 
>   vcs/widgets/vcscommitdialog.h 85f8d59 
>   vcs/widgets/vcsdiffpatchsources.h 6ca06f8 
>   vcs/widgets/vcsdiffwidget.h 088b073 
>   vcs/widgets/vcsimportmetadatawidget.h c088e39 
>   vcs/widgets/vcslocationwidget.h 1d921a0 
> 
> Diff: https://git.reviewboard.kde.org/r/124427/diff/
> 
> 
> Testing
> -------
> 
> Builds, passes tests
> 
> 
> Thanks,
> 
> Boris Egorov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150724/441ade3d/attachment-0001.html>

More information about the KDevelop-devel mailing list