Fix lint issues in RocksJava related files

[sagar0]

Ran make format to fix lint issue. There are no other changes in this PR other than the ones done by make format.
Most were pointed by the internal linter for #4833, but there were too many to just fix/point out in that great contribution to get RocksJava API on par with the C++ API. Hence fixing separately.

Test Plan:

• make check
• make rocksdbjava jtest

[facebook-github-bot]

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[adamretter]

@sagar0 I really dislike this style of indenting on the JNI signature declarations. I am not sure it makes sense as the method names will always be very very long. Ultimately this leads to the downside of having one variable per line, difficult to read code and no benefit.

Is there any chance to get an exclusion for this rule for CPP code inside java/rocksjni?

[sagar0]

@adamretter We are just using the clang formatter. make format uses format-diff.sh which internally calls clang-format. (Our internal linter is much more stricter than the formatter, whining on other rules.)
While we could potentially have an exclusion for the files under java directory, we will lose the efficiency of formatting everything with a tool, leading to many different formatting styles from different contributors based on personal preferences. I am open to suggestions.

[sagar0]

I am not sure it makes sense as the method names will always be very very long. Ultimately this leads to the downside of having one variable per line, difficult to read code and no benefit.

Clang format does not always put one arg per line (if this is your main concern). You can see that when there are many params, clang chooses a different style of putting multiple args on the same line. Example:

rocksdb/db/builder.cc

Lines 65 to 82 in bb474e9

	Status BuildTable(
	const std::string& dbname, Env* env, const ImmutableCFOptions& ioptions,
	const MutableCFOptions& mutable_cf_options, const EnvOptions& env_options,
	TableCache* table_cache, InternalIterator* iter,
	std::vector<std::unique_ptr<FragmentedRangeTombstoneIterator>>
	range_del_iters,
	FileMetaData* meta, const InternalKeyComparator& internal_comparator,
	const std::vector<std::unique_ptr<IntTblPropCollectorFactory>>*
	int_tbl_prop_collector_factories,
	uint32_t column_family_id, const std::string& column_family_name,
	std::vector<SequenceNumber> snapshots,
	SequenceNumber earliest_write_conflict_snapshot,
	SnapshotChecker* snapshot_checker, const CompressionType compression,
	const CompressionOptions& compression_opts, bool paranoid_file_checks,
	InternalStats* internal_stats, TableFileCreationReason reason,
	EventLogger* event_logger, int job_id, const Env::IOPriority io_priority,
	TableProperties* table_properties, int level, const uint64_t creation_time,
	const uint64_t oldest_key_time, Env::WriteLifeTimeHint write_hint) {

[adamretter]

My point was that it does put each variable on a separate line when the method name is very very long which is typically only the case in the JNI C++ code. The example you provided doesn't show this because the method name is very short.

I don't want to turn off formatting for the JNI C++ code, I just want to tweak it for that folder so that we have sensible formatting that is both readable and usable. Is it possible to configure the clang format rules for RocksDB or add exceptions for only certain rules?

[sagar0]

I am still not convinced that we should tweak the formatting for the JNI code. There are many places in the current C++ API code where each variable is placed on a separate line, as the method name sometimes becomes long when prefixed with the class name; granted it could be a little more so in the JNI C++ code. But the actual style rule causing this seems sane enough to me

In case if it is decided to tweak, AlignAfterOpenBracket could be set to DontAlign or AlwaysBreak for JNI files in our .clang-format, I believe (I am not an expert on style-rules, just googled my way to it).

[facebook-github-bot]

@sagar0 has updated the pull request. Re-import the pull request

[facebook-github-bot]

@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[siying]

Do we plan to move forward, or do we plan to abandon it?

[sagar0]

Abandoned, as there was no consensus.