Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

update docs#1054

Merged
lukaszstolarczuk merged 3 commits intopmem:masterfrom
lukaszstolarczuk:fix-stage-docs
Mar 19, 2021
Merged

update docs#1054
lukaszstolarczuk merged 3 commits intopmem:masterfrom
lukaszstolarczuk:fix-stage-docs

Conversation

@lukaszstolarczuk
Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk commented Mar 8, 2021

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 8, 2021

Codecov Report

Merging #1054 (051c696) into master (eca8f5f) will decrease coverage by 0.16%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
- Coverage   94.25%   94.09%   -0.17%     
==========================================
  Files          48       48              
  Lines        4720     4742      +22     
==========================================
+ Hits         4449     4462      +13     
- Misses        271      280       +9     
Flag Coverage Δ
tests_clang_debug_cpp17 93.58% <75.00%> (-0.11%) ⬇️
tests_gcc_debug 91.96% <75.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/libpmemobj++/pool.hpp 87.05% <ø> (+1.16%) ⬆️
include/libpmemobj++/transaction.hpp 79.83% <66.66%> (-2.74%) ⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 94.20% <100.00%> (ø)
include/libpmemobj++/condition_variable.hpp 73.80% <0.00%> (-4.77%) ⬇️
...libpmemobj++/detail/enumerable_thread_specific.hpp 98.91% <0.00%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eca8f5f...051c696. Read the comment docs.

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)


include/libpmemobj++/transaction.hpp, line 456 at r1 (raw file):

		https://pmem.io/pmdk/manpages/linux/master/libpmemobj/pmemobj_tx_begin.3
	 */
	enum class stage {

actually I can see we only use it in register_callback... why not elsewhere?

@karczex
Copy link
Copy Markdown

karczex commented Mar 12, 2021


include/libpmemobj++/transaction.hpp, line 456 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

actually I can see we only use it in register_callback... why not elsewhere?

that's really good point :)
and it seems, such change would be pretty simple sed

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukaszstolarczuk)

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @igchor)


include/libpmemobj++/transaction.hpp, line 456 at r1 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

that's really good point :)
and it seems, such change would be pretty simple sed

@igchor, what do you think? Do you know if there was a reason for not using this...?


include/libpmemobj++/container/concurrent_hash_map.hpp, line 1968 at r2 (raw file):

		if (layout_features.incompat != header_features().incompat)
			throw pmem::layout_error(
				"Incompat flags mismatch, for more details go to: https://pmem.io/libpmemobj-cpp\n");

hmm I tried to find some better link - what you think, should we put here something more precise...?

@karczex
Copy link
Copy Markdown

karczex commented Mar 15, 2021


include/libpmemobj++/container/concurrent_hash_map.hpp, line 1968 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

hmm I tried to find some better link - what you think, should we put here something more precise...?

Are you sure it's good idea to put link to documentation in error in low level library?

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @igchor and @karczex)


include/libpmemobj++/container/concurrent_hash_map.hpp, line 1968 at r2 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

Are you sure it's good idea to put link to documentation in error in low level library?

maybe you're right, maybe we should remove this and we have no problem...? :)

@lukaszstolarczuk lukaszstolarczuk changed the title update stage enum's docs update docs Mar 17, 2021
so it will be available in the generated docs.
Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)


include/libpmemobj++/pool.hpp, line 457 at r3 (raw file):

Quoted 9 lines of code…
#ifndef _WIN32
	/**
	 * Default create mode
	 */
	static const int DEFAULT_MODE = S_IWUSR | S_IRUSR;
#else
	/**
	 * Default create mode
	 */

IMO useless change, like this comments which only duplicate variable name.

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor, @karczex, and @KFilipek)


include/libpmemobj++/pool.hpp, line 457 at r3 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…
#ifndef _WIN32
	/**
	 * Default create mode
	 */
	static const int DEFAULT_MODE = S_IWUSR | S_IRUSR;
#else
	/**
	 * Default create mode
	 */

IMO useless change, like this comments which only duplicate variable name.

without this change all user can see is in our docs is that we use some DEFAULT_MODE (see: https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1pool.html#a8c7a780cfb3bc6c708856783938a9e8c ).
With this change they know exactly what this const equals to.

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)


include/libpmemobj++/transaction.hpp, line 456 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

@igchor, what do you think? Do you know if there was a reason for not using this...?

You mean in places where we for example check pmemobj_tx_stage() == TX_STAGE_WORK? It's beacuse pmemobj_tx_stage reutrns an int and we would have to cast the enum. In general, we should implement some C++ method like stage() within the transaction class.


include/libpmemobj++/container/concurrent_hash_map.hpp, line 1968 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

maybe you're right, maybe we should remove this and we have no problem...? :)

This was done so that to offer some kind of information for users of older libraries. If there is new incompat flag introduced we can add some meanigful message in the new version of the library but for older versions, we cannot - we can only point to some external documenation.

Copy link
Copy Markdown
Member Author

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KFilipek)


include/libpmemobj++/container/concurrent_hash_map.hpp, line 1968 at r2 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

This was done so that to offer some kind of information for users of older libraries. If there is new incompat flag introduced we can add some meanigful message in the new version of the library but for older versions, we cannot - we can only point to some external documenation.

ok, thx

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again :lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lukaszstolarczuk)

@lukaszstolarczuk lukaszstolarczuk merged commit 2d0c864 into pmem:master Mar 19, 2021
@lukaszstolarczuk lukaszstolarczuk deleted the fix-stage-docs branch March 19, 2021 10:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants