[no subject]
This version addresses all the feedback so far (Thanks!). The largest change is support for using [[musttail]] in C23, not just C++. -Andi
[no subject]
Greetings, I noticed you from LinkedIn? Can I share an idea on this email?
[no subject]
This patch set is not actually particulalry new, I have been maintaining
it locally one Darwin branches and it has been tested on several versions
of Darwin both with and without Alex's __has_{feature, extension} patch.
This is one of the three most significant blockers to importing the macOS
SDKs properly, and cannot currently be fixincludes-ed (in fact it can not
ever really since the attribute is uaer-facing and so can be in end-user
code that we cannot fix).
OK for trunk?
thanks
Iain
[no subject]
From: Kito Cheng Reply-To: Subject: [PATCH v1 0/4] RISC-V target attribute In-Reply-To: This patch set implement target attribute for RISC-V target, which is similar to other target like x86 or ARM, let user able to set some local setting per function without changing global settings. We support arch, tune and cpu first, and we will support other target attribute later, this version DOES NOT include multi-version function support yet, that is future work, probably work for GCC 15. The full proposal is put in RISC-V C-API document[1], which has discussed with RISC-V LLVM community, so we have consistent syntax and semantics. [1] https://github.com/riscv-non-isa/riscv-c-api-doc/pull/35
[no subject]
Hello, Hope you are doing well with the family. I noticed on LinkedIn. Can I share an idea here?
[no subject]
>From d57ac4f9a095a2f616863efd524ac2d87276becb Mon Sep 17 00:00:00 2001
From: Eddy Young
Date: Sun, 13 Aug 2023 19:59:12 +0100
Subject: [PATCH] gcc/reload.h: Change type of x_spill_indirect_levels
---
ChangeLog| 5 +
gcc/reload.h | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index 3dd1ce544af..442aa9192a9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-08-13 Eddy Young
+
+ * gcc/reload.h: Change type of x_spill_indirect_levels of struct
+ target_reload to support C++17 build.
+
2015-06-23 Release Manager
* GCC 4.8.5 released.
diff --git a/gcc/reload.h b/gcc/reload.h
index 7a13ad30e82..1e94d8ea93b 100644
--- a/gcc/reload.h
+++ b/gcc/reload.h
@@ -166,7 +166,7 @@ struct target_reload {
value indicates the level of indirect addressing supported, e.g., two
means that (MEM (MEM (REG n))) is also valid if (REG n) does not get
a hard register. */
- bool x_spill_indirect_levels;
+ unsigned char x_spill_indirect_levels;
/* True if caller-save has been reinitialized. */
bool x_caller_save_initialized_p;
--
2.39.2
[no subject]
>From 104178c3912314f41a61a316e7c3bc0487ea8f3c Mon Sep 17 00:00:00 2001
From: K1ZeKaTo
Date: Sat, 5 Aug 2023 14:50:19 +0000
Subject: [PATCH] Make the rvalue work fine.
It seems not considering the rvalue in the last version.
---
libstdc++-v3/include/bits/iterator_concepts.h | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/libstdc++-v3/include/bits/iterator_concepts.h
b/libstdc++-v3/include/bits/iterator_concepts.h
index e32e94dc9fc..8bb7c0fb8c0 100644
--- a/libstdc++-v3/include/bits/iterator_concepts.h
+++ b/libstdc++-v3/include/bits/iterator_concepts.h
@@ -86,14 +86,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
concept __can_reference = requires { typename __with_ref<_Tp>; };
template
- concept __dereferenceable = requires(_Tp& __t)
+ concept __dereferenceable = requires(_Tp&& __t)
{
- { *__t } -> __can_reference;
+ { *static_cast<_Tp&&>(__t) } -> __can_reference;
};
} // namespace __detail
template<__detail::__dereferenceable _Tp>
-using iter_reference_t = decltype(*std::declval<_Tp&>());
+using iter_reference_t = decltype(*std::declval<_Tp&&>());
namespace ranges
{
@@ -116,7 +116,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template
requires __adl_imove<_Tp>
struct __result<_Tp>
- { using type = decltype(iter_move(std::declval<_Tp>())); };
+ { using type = decltype(iter_move(std::declval<_Tp&&>())); };
template
requires (!__adl_imove<_Tp>)
@@ -129,9 +129,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_S_noexcept()
{
if constexpr (__adl_imove<_Tp>)
- return noexcept(iter_move(std::declval<_Tp>()));
+ return noexcept(iter_move(std::declval<_Tp&&>()));
else
- return noexcept(*std::declval<_Tp>());
+ return noexcept(*std::declval<_Tp&&>());
}
public:
@@ -148,9 +148,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
if constexpr (__adl_imove<_Tp>)
return iter_move(static_cast<_Tp&&>(__e));
else if constexpr (is_lvalue_reference_v>)
- return static_cast<__type<_Tp>>(*__e);
+ return static_cast<__type<_Tp>>(*static_cast<_Tp&&>(__e));
else
- return *__e;
+ return *static_cast<_Tp&&>(__e);
}
};
} // namespace __cust_imove
--
2.41.0
[no subject]
Hi,
Below is the fix to regression bug
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110198
Was bootstrapped and regtested successfully on x86_64-linux-gnu
Considering mishap from last patch, I'd would appreciate if you could
also regtest it, to be sure :)
Thanks,
Benjamin.
>From 04186f04a3f172d7ccf9824cc71faca489eb39af Mon Sep 17 00:00:00 2001
From: benjamin priour
Date: Thu, 22 Jun 2023 21:39:05 +0200
Subject: [PATCH] [PATCH] analyzer: Fix regression bug after
r14-1632-g9589a46ddadc8b [pr110198]
g++.dg/analyzer/pr100244.C was failing after a patch of PR109439.
The reason was a spurious preemptive return of get_store_value upon
out-of-bounds read that
was preventing further checks. Now instead, a boolean value check_poisoned goes
to false when
a OOB is detected, and is later on given to get_or_create_initial_value.
gcc/analyzer/ChangeLog:
* region-model-manager.cc
(region_model_manager::get_or_create_initial_value): Take an
optional boolean value to bypass poisoning checks
* region-model-manager.h: Update declaration of the above function.
* region-model.cc (region_model::get_store_value): No longer
returns on OOB, but rather gives a boolean to
get_or_create_initial_value.
(region_model::check_region_access): Update docstring.
(region_model::check_region_for_write): Update docstring.
Signed-off-by: benjamin priour
---
gcc/analyzer/region-model-manager.cc | 5 +++--
gcc/analyzer/region-model-manager.h | 3 ++-
gcc/analyzer/region-model.cc | 15 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index 1453acf7bc9..4f11ef4bd29 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -293,9 +293,10 @@ region_model_manager::create_unique_svalue (tree type)
necessary. */
const svalue *
-region_model_manager::get_or_create_initial_value (const region *reg)
+region_model_manager::get_or_create_initial_value (const region *reg,
+ bool check_poisoned)
{
- if (!reg->can_have_initial_svalue_p ())
+ if (!reg->can_have_initial_svalue_p () && check_poisoned)
return get_or_create_poisoned_svalue (POISON_KIND_UNINIT,
reg->get_type ());
diff --git a/gcc/analyzer/region-model-manager.h
b/gcc/analyzer/region-model-manager.h
index 3340c3ebd1e..ff5333bf07c 100644
--- a/gcc/analyzer/region-model-manager.h
+++ b/gcc/analyzer/region-model-manager.h
@@ -49,7 +49,8 @@ public:
tree type);
const svalue *get_or_create_poisoned_svalue (enum poison_kind kind,
tree type);
- const svalue *get_or_create_initial_value (const region *reg);
+ const svalue *get_or_create_initial_value (const region *reg,
+bool check_poisoned = true);
const svalue *get_ptr_svalue (tree ptr_type, const region *pointee);
const svalue *get_or_create_unaryop (tree type, enum tree_code op,
const svalue *arg);
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 6bc60f89f3d..187013a37cc 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -2373,8 +2373,9 @@ region_model::get_store_value (const region *reg,
if (reg->empty_p ())
return m_mgr->get_or_create_unknown_svalue (reg->get_type ());
+ bool check_poisoned = true;
if (check_region_for_read (reg, ctxt))
-return m_mgr->get_or_create_unknown_svalue(reg->get_type());
+check_poisoned = false;
/* Special-case: handle var_decls in the constant pool. */
if (const decl_region *decl_reg = reg->dyn_cast_decl_region ())
@@ -2427,7 +2428,7 @@ region_model::get_store_value (const region *reg,
== RK_GLOBALS)
return get_initial_value_for_global (reg);
- return m_mgr->get_or_create_initial_value (reg);
+ return m_mgr->get_or_create_initial_value (reg, check_poisoned);
}
/* Return false if REG does not exist, true if it may do.
@@ -2790,7 +2791,7 @@ region_model::get_string_size (const region *reg) const
/* If CTXT is non-NULL, use it to warn about any problems accessing REG,
using DIR to determine if this access is a read or write.
- Return TRUE if an UNKNOWN_SVALUE needs be created.
+ Return TRUE if an OOB access was detected.
If SVAL_HINT is non-NULL, use it as a hint in diagnostics
about the value that would be written to REG. */
@@ -2804,10 +2805,10 @@ region_model::check_region_access (const region *reg,
if (!ctxt)
return false;
- bool need_unknown_sval = false;
+ bool oob_access_detected = false;
check_region_for_taint (reg, dir, ctxt);
if (!check_region_bounds (reg, dir, sval_hint, ctxt))
-need_unknown_
[no subject]
Dear all,
the compile-time simplification of intrinsic SET_EXPONENT was
broken since the early days of gfortran for argument X < 1
(including negative X) and for I < 0. I identified and fixed
several issues in the implementation. The testcase explores
argument space comparing compile-time and runtime results and
is checked against Intel.
Regtested on x86_64-pc-linux-gnu. OK for mainline?
This is not a regression, but can lead to wrong code.
Would it be OK to backport to open branches?
Thanks,
Harald
From fa4cb42870df60debdbd51e2ddc6d6ab9e6a Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Fri, 14 Apr 2023 20:45:19 +0200
Subject: [PATCH] Fortran: fix compile-time simplification of SET_EXPONENT
[PR109511]
gcc/fortran/ChangeLog:
PR fortran/109511
* simplify.cc (gfc_simplify_set_exponent): Fix implementation of
compile-time simplification of intrinsic SET_EXPONENT for argument
X < 1 and for I < 0.
gcc/testsuite/ChangeLog:
PR fortran/109511
* gfortran.dg/set_exponent_1.f90: New test.
---
gcc/fortran/simplify.cc | 12 +++
gcc/testsuite/gfortran.dg/set_exponent_1.f90 | 36
2 files changed, 42 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/set_exponent_1.f90
diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc
index ecf0e3558df..b65854c1021 100644
--- a/gcc/fortran/simplify.cc
+++ b/gcc/fortran/simplify.cc
@@ -7364,7 +7364,7 @@ gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
{
gfc_expr *result;
mpfr_t exp, absv, log2, pow2, frac;
- unsigned long exp2;
+ long exp2;
if (x->expr_type != EXPR_CONSTANT || i->expr_type != EXPR_CONSTANT)
return NULL;
@@ -7396,19 +7396,19 @@ gfc_simplify_set_exponent (gfc_expr *x, gfc_expr *i)
mpfr_abs (absv, x->value.real, GFC_RND_MODE);
mpfr_log2 (log2, absv, GFC_RND_MODE);
- mpfr_trunc (log2, log2);
+ mpfr_floor (log2, log2);
mpfr_add_ui (exp, log2, 1, GFC_RND_MODE);
/* Old exponent value, and fraction. */
mpfr_ui_pow (pow2, 2, exp, GFC_RND_MODE);
- mpfr_div (frac, absv, pow2, GFC_RND_MODE);
+ mpfr_div (frac, x->value.real, pow2, GFC_RND_MODE);
/* New exponent. */
- exp2 = (unsigned long) mpz_get_d (i->value.integer);
- mpfr_mul_2exp (result->value.real, frac, exp2, GFC_RND_MODE);
+ exp2 = mpz_get_si (i->value.integer);
+ mpfr_mul_2si (result->value.real, frac, exp2, GFC_RND_MODE);
- mpfr_clears (absv, log2, pow2, frac, NULL);
+ mpfr_clears (absv, log2, exp, pow2, frac, NULL);
return range_check (result, "SET_EXPONENT");
}
diff --git a/gcc/testsuite/gfortran.dg/set_exponent_1.f90 b/gcc/testsuite/gfortran.dg/set_exponent_1.f90
new file mode 100644
index 000..4c063e8330b
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/set_exponent_1.f90
@@ -0,0 +1,36 @@
+! { dg-do run }
+! PR fortran/109511
+! Check compile-time simplification of SET_EXPONENT against runtime
+
+program exponent
+ implicit none
+ integer :: i
+ i = 0
+ print *, i, set_exponent(1., 0), set_exponent(1., i)
+ if (set_exponent(1., 0) /= set_exponent(1., i)) stop 1
+ i = 1
+ print *, i, set_exponent(1., 1), set_exponent(1., i)
+ if (set_exponent(1., 1) /= set_exponent(1., i)) stop 2
+ i = 2
+ print *, i, set_exponent(-1.75, 2), set_exponent(-1.75, i)
+ if (set_exponent(-1.75, 2) /= set_exponent(-1.75, i)) stop 3
+ print *, i, set_exponent(0.1875, 2), set_exponent(0.1875, i)
+ if (set_exponent(0.1875, 2) /= set_exponent(0.1875, i)) stop 4
+ i = 3
+ print *, i, set_exponent(0.75, 3), set_exponent(0.75, i)
+ if (set_exponent(0.75, 3) /= set_exponent(0.75, i)) stop 5
+ i = 4
+ print *, i, set_exponent(-2.5, 4), set_exponent(-2.5, i)
+ if (set_exponent(-2.5, 4) /= set_exponent(-2.5, i)) stop 6
+ i = -1
+ print *, i, set_exponent(1., -1), set_exponent(1., i)
+ if (set_exponent(1., -1) /= set_exponent(1., i)) stop 7
+ i = -2
+ print *, i, set_exponent(1.125, -2), set_exponent(1.125, i)
+ if (set_exponent(1.125, -2) /= set_exponent(1.125, i)) stop 8
+ print *, i, set_exponent(-0.25, -2), set_exponent(-0.25, i)
+ if (set_exponent(-0.25, -2) /= set_exponent(-0.25, i)) stop 9
+ i = -3
+ print *, i, set_exponent(0.75, -3), set_exponent(0.75, i)
+ if (set_exponent(0.75, -3) /= set_exponent(0.75, i)) stop 10
+end program exponent
--
2.35.3
[PATCH (pushed)] gcc-changelog: support digits in PR's component in subject
Yes, fixed in the following patch.
Martin
contrib/ChangeLog:
* gcc-changelog/git_commit.py: Support digits in PR's
component in subject.
---
contrib/gcc-changelog/git_commit.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index 7fde02cba85..b73e587eb98 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -163,7 +163,7 @@ author_line_regex = \
re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.* <.*>)')
additional_author_regex = re.compile(r'^\t(?P\ *)?(?P.* <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
-subject_pr_regex =
re.compile(r'(^|\W)PR\s+(?P[a-zA-Z+-]+)/(?P\d{4,7})')
+subject_pr_regex =
re.compile(r'(^|\W)PR\s+(?P[a-zA-Z0-9+-]+)/(?P\d{4,7})')
subject_pr2_regex = re.compile(r'[(\[]PR\s*(?P\d{4,7})[)\]]')
pr_regex = re.compile(r'\tPR (?P[a-z0-9+-]+\/)?(?P[0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
--
2.39.0
[PATCH][pushed] mklog: fill-up subject prefix only for a single PR
I've just pushed this one.
Martin
contrib/ChangeLog:
* mklog.py: Use component: [PR xyz] only when one PR is used.
---
contrib/mklog.py | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/contrib/mklog.py b/contrib/mklog.py
index cd5ef0bcc74..8693767a7f6 100755
--- a/contrib/mklog.py
+++ b/contrib/mklog.py
@@ -73,8 +73,6 @@ PATCH must be generated using diff(1)'s -up or -cp options
script_folder = os.path.realpath(__file__)
root = os.path.dirname(os.path.dirname(script_folder))
-firstpr = ''
-
def find_changelog(path):
folder = os.path.split(path)[0]
@@ -157,12 +155,13 @@ def get_rel_path_if_prefixed(path, folder):
def generate_changelog(data, no_functions=False, fill_pr_titles=False,
additional_prs=None):
+global prs
+prs = []
+
changelogs = {}
changelog_list = []
-prs = []
out = ''
diff = PatchSet(data)
-global firstpr
if additional_prs:
for apr in additional_prs:
@@ -212,9 +211,6 @@ def generate_changelog(data, no_functions=False,
fill_pr_titles=False,
if pr not in this_file_prs and pr2 not in prs:
prs.append(pr2)
-if prs:
-firstpr = prs[0]
-
if fill_pr_titles:
out += get_pr_titles(prs)
@@ -372,11 +368,12 @@ if __name__ == '__main__':
end = lines[len(start):]
with open(args.changelog, 'w') as f:
if not start or not start[0]:
-# initial commit subject line 'component: [PRn]'
-m = prnum_regex.match(firstpr)
-if m:
-title = f'{m.group("comp")}: [PR{m.group("num")}]'
-start.insert(0, title)
+if len(prs) == 1:
+ # initial commit subject line 'component: [PRn]'
+m = prnum_regex.match(prs[0])
+if m:
+title = f'{m.group("comp")}: [PR{m.group("num")}]'
+start.insert(0, title)
if start:
# append empty line
if start[-1] != '':
--
2.37.1
[no subject]
Hi!
On 2022-03-04T14:46:25+0100, I wrote:
> Pushed to master branch commit 8935589b496f755e08cadf26d8ceddf0dd6e0968
> "OMP lowering: Regimplify 'OMP_CLAUSE_MAP_DECL_MAKE_ADDRESSABLE' DECLs
> [PR100280, PR104132, PR104133]", see attached.
> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c
> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c
> [...]
> @@ -27,8 +31,12 @@ int main()
>(volatile int *)
> #define N 123
>int b[N] = { 0 };
> + unsigned long long f1;
> + /*TODO See above. */
> + (volatile void *)
Ah, the famous last-minute change just before 'git push'... To work
around execution failure with GCN offloading, we're explicitly making
'f1' addressable here -- but I didn't realize that this also affects
diagnostics, sorry.
Pushed to master branch commit 14dfbb53594e164fe222476523a68039a8bd5252
"Fix 'libgomp.oacc-c-c++-common/kernels-decompose-1.c' expected
diagnostics", see attached.
Grüße
Thomas
>
> #pragma acc kernels /* { dg-line l_compute[incr c_compute] } */
> + /* { dg-note {variable 'g2\.0' declared in block isn't candidate for
> adjusting OpenACC privatization level: not addressable} {} { target *-*-* }
> l_compute$c_compute } */
>{
> [...]
> +/* { dg-note {beginning 'gang-single' part in OpenACC 'kernels' region}
> {} { target *-*-* } .+1 } */
> + f1 = 1;
> + /* { dg-note {forwarded loop nest in OpenACC 'kernels' region to
> 'parloops' for analysis} {} { target *-*-* } .+1 } */
> +#pragma acc loop /* { dg-line l_loop_c[incr c_loop_c] } */
> + /* { dg-note {variable 'c' in 'private' clause is candidate for
> adjusting OpenACC privatization level} {} { target *-*-* } l_loop_c$c_loop_c
> } */
> + /* { dg-optimized {assigned OpenACC seq loop parallelism} {} { target
> *-*-* } l_loop_c$c_loop_c } */
> + for (c = 20; c > 0; --c)
> + f1 *= c;
> +
> + /* { dg-note {beginning 'parloops' part in OpenACC 'kernels' region}
> {} { target *-*-* } .+1 } */
> + if (c != 234)
> + __builtin_abort ();
> + /* { dg-optimized {assigned OpenACC seq loop parallelism} {} { target
> *-*-* } l_compute$c_compute } */
> +}
>}
> [...]
> + assert (f1 == 243290200817664ULL);
>
>return 0;
> }
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
>From 14dfbb53594e164fe222476523a68039a8bd5252 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge
Date: Fri, 4 Mar 2022 20:34:40 +0100
Subject: [PATCH] Fix 'libgomp.oacc-c-c++-common/kernels-decompose-1.c'
expected diagnostics
Fix-up for recent commit 8935589b496f755e08cadf26d8ceddf0dd6e0968
"OMP lowering: Regimplify 'OMP_CLAUSE_MAP_DECL_MAKE_ADDRESSABLE' DECLs
[PR100280, PR104132, PR104133]": adjust for a GCN offloading workaround
added just before commit: '(volatile void *) '.
PR testsuite/104791
libgomp/
* testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c: Fix
expected diagnostics.
---
.../testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c
index 049b3a44b03..985a547d381 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/kernels-decompose-1.c
@@ -37,6 +37,8 @@ int main()
#pragma acc kernels /* { dg-line l_compute[incr c_compute] } */
/* { dg-note {variable 'g2\.0' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { target *-*-* } l_compute$c_compute } */
+ /* { dg-note {variable 'f1\.1' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { target *-*-* } l_compute$c_compute } */
+ /* { dg-note {variable 'f1\.2' declared in block isn't candidate for adjusting OpenACC privatization level: not addressable} {} { target *-*-* } l_compute$c_compute } */
{
/* { dg-note {beginning 'gang-single' part in OpenACC 'kernels' region} {} { target *-*-* } .+1 } */
int c = 234;
--
2.25.1
Re: [PATCH RFC] mklog: add subject line skeleton
On 6/17/21 5:17 AM, Martin Liška wrote: On 6/17/21 5:42 AM, Jason Merrill wrote: |Does this seem like an improvement?| Yes, I support the patch. I've made some adjustments to the patch in order to make flake8 happy. Pushed with those changes, thanks. Jason
Re: [PATCH RFC] mklog: add subject line skeleton
On 6/17/21 5:42 AM, Jason Merrill wrote:
|Does this seem like an improvement?|
Yes, I support the patch.
I've made some adjustments to the patch in order to make flake8 happy.
Martin
diff --git a/contrib/mklog.py b/contrib/mklog.py
index 5c93c707128..1f59055e723 100755
--- a/contrib/mklog.py
+++ b/contrib/mklog.py
@@ -39,6 +39,7 @@ import requests
from unidiff import PatchSet
pr_regex = re.compile(r'(\/(\/|\*)|[Cc*!])\s+(?PPR [a-z+-]+\/[0-9]+)')
+prnum_regex = re.compile(r'PR (?P[a-z+-]+)/(?P[0-9]+)')
dr_regex = re.compile(r'(\/(\/|\*)|[Cc*!])\s+(?PDR [0-9]+)')
dg_regex = re.compile(r'{\s+dg-(error|warning)')
identifier_regex = re.compile(r'^([a-zA-Z0-9_#].*)')
@@ -67,6 +68,8 @@ PATCH must be generated using diff(1)'s -up or -cp options
script_folder = os.path.realpath(__file__)
root = os.path.dirname(os.path.dirname(script_folder))
+firstpr = ''
+
def find_changelog(path):
folder = os.path.split(path)[0]
@@ -134,6 +137,7 @@ def generate_changelog(data, no_functions=False, fill_pr_titles=False):
prs = []
out = ''
diff = PatchSet(data)
+global firstpr
for file in diff:
# skip files that can't be parsed
@@ -166,6 +170,9 @@ def generate_changelog(data, no_functions=False, fill_pr_titles=False):
# Found dg-warning/dg-error line
break
+if prs:
+firstpr = prs[0]
+
if fill_pr_titles:
out += get_pr_titles(prs)
@@ -308,8 +315,14 @@ if __name__ == '__main__':
start = list(takewhile(lambda l: not l.startswith('#'), lines))
end = lines[len(start):]
with open(args.changelog, 'w') as f:
+if not start or not start[0]:
+# initial commit subject line 'component: [PRn]'
+m = prnum_regex.match(firstpr)
+if m:
+title = f'{m.group("comp")}: [PR{m.group("num")}]'
+start.insert(0, title)
if start:
-# appent empty line
+# append empty line
if start[-1] != '':
start.append('')
else:
[PATCH RFC] mklog: add subject line skeleton
In the recent gcc-commit-mklog thread on gcc@ it occurred to me that the
command could also fill in part of the commit subject line. If the first PR is
foo/1234, and the commit does not yet have a subject line, this will add
foo: [PR1234]
Does this seem like an improvement?
contrib/ChangeLog:
* mklog.py: Add an initial component: [PRn] line when
we have a PR.
---
contrib/mklog.py | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/contrib/mklog.py b/contrib/mklog.py
index 5c93c707128..6113ae30a8e 100755
--- a/contrib/mklog.py
+++ b/contrib/mklog.py
@@ -39,6 +39,7 @@ import requests
from unidiff import PatchSet
pr_regex = re.compile(r'(\/(\/|\*)|[Cc*!])\s+(?PPR [a-z+-]+\/[0-9]+)')
+prnum_regex = re.compile(r'PR (?P[a-z+-]+)/(?P[0-9]+)')
dr_regex = re.compile(r'(\/(\/|\*)|[Cc*!])\s+(?PDR [0-9]+)')
dg_regex = re.compile(r'{\s+dg-(error|warning)')
identifier_regex = re.compile(r'^([a-zA-Z0-9_#].*)')
@@ -67,6 +68,7 @@ PATCH must be generated using diff(1)'s -up or -cp options
script_folder = os.path.realpath(__file__)
root = os.path.dirname(os.path.dirname(script_folder))
+firstpr = ''
def find_changelog(path):
folder = os.path.split(path)[0]
@@ -134,6 +136,7 @@ def generate_changelog(data, no_functions=False,
fill_pr_titles=False):
prs = []
out = ''
diff = PatchSet(data)
+global firstpr;
for file in diff:
# skip files that can't be parsed
@@ -166,6 +169,9 @@ def generate_changelog(data, no_functions=False,
fill_pr_titles=False):
# Found dg-warning/dg-error line
break
+if prs:
+firstpr = prs[0]
+
if fill_pr_titles:
out += get_pr_titles(prs)
@@ -308,8 +314,14 @@ if __name__ == '__main__':
start = list(takewhile(lambda l: not l.startswith('#'), lines))
end = lines[len(start):]
with open(args.changelog, 'w') as f:
+if (not start) or (start[0] == ''):
+# initial commit subject line 'component: [PRn]'
+m = prnum_regex.match(firstpr)
+if m:
+start.insert (0, m.group('comp') + ': [PR'
+ + m.group('num') + ']')
if start:
-# appent empty line
+# append empty line
if start[-1] != '':
start.append('')
else:
base-commit: ff4deb4b1d0c5947669ddc76fde4db83e28009d8
--
2.27.0
[no subject]
I.n.h. Module metadata package name: com.google.android.modulemetadatacan't find the pid Failed to find: /data/misc/anrd/ Duration of 'DUMPSYS CRITICAL': 1.25s Duration of 'DUMP TRACES': 46.26s Adding dir /cache/recovery (recursive: 1) Adding dir /data/misc/recovery (recursive: 1) Adding dir /data/misc/update_engine_log (recursive: 1) Adding dir /data/misc/logd (recursive: 0) /data/misc/logd: No such file or directory Unable to read link for /proc/24763/ns/mnt: No such file or directory MOUNT INFO: 46 entries added to zip file execvp on command 'iotop -n 1 -m 100' failed (error: No such file or directory) *** command 'iotop -n 1 -m 100' failed: exit code 1 Duration of 'CPU INFO': 4.17s Duration of 'PROCESSES AND THREADS': 1.50s Warning: Skipping "android.frameworks.cameraservice.service@2.0::ICameraService/default": cannot be fetched from service manager (null) Warning: Skipping "android.frameworks.displayservice@1.0::IDisplayService/default": cannot be fetched from service manager (null) Warning: Skipping "android.frameworks.schedulerservice@1.0::ISchedulingPolicyService/default": cannot be fetched from service manager (null) Warning: Skipping "android.frameworks.sensorservice@1.0::ISensorManager/default": cannot be fetched from service manager (null) Warning: Skipping "android.frameworks.stats@1.0::IStats/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.atrace@1.0::IAtraceDevice/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.audio.effect@5.0::IEffectsFactory/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.audio@5.0::IDevicesFactory/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.biometrics.fingerprint@2.1::IBiometricsFingerprint/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.bluetooth.audio@2.0::IBluetoothAudioProvidersFactory/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.bluetooth@1.0::IBluetoothHci/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.boot@1.0::IBootControl/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.camera.provider@2.4::ICameraProvider/legacy/0": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.cas@1.0::IMediaCasService/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.cas@1.1::IMediaCasService/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.configstore@1.0::ISurfaceFlingerConfigs/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.configstore@1.1::ISurfaceFlingerConfigs/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::ICryptoFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::ICryptoFactory/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::ICryptoFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::IDrmFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::IDrmFactory/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.0::IDrmFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.1::ICryptoFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.1::ICryptoFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.1::IDrmFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.1::IDrmFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.2::ICryptoFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.2::ICryptoFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.2::IDrmFactory/clearkey": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.drm@1.2::IDrmFactory/widevine": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.gatekeeper@1.0::IGatekeeper/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.gnss@1.0::IGnss/default": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.gnss@1.0::IGnss/gnss_vendor": cannot be fetched from service manager (null) Warning: Skipping "android.hardware.gnss@1.1::IGnss/default": cannot be fetched from service
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On Sun, 13 Jun 2021 at 06:51, Tobias Burnus wrote: > > On 11.06.21 09:43, Martin Liška wrote: > > I support the patch, please install it (except the not intentional change > > in gcc/c/c-parser.c). > Committed as r12-1408-gd554f43c98eb07f222afef5e90b5582d65519f7e > > Then, please ask Jonathan for updating of the server hook. > > @Jonathan: ↑ - thanks. That's done now.
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 11.06.21 09:43, Martin Liška wrote: I support the patch, please install it (except the not intentional change in gcc/c/c-parser.c). Committed as r12-1408-gd554f43c98eb07f222afef5e90b5582d65519f7e Then, please ask Jonathan for updating of the server hook. @Jonathan: ↑ - thanks. Tobais - Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 6/10/21 5:32 PM, Tobias Burnus wrote: On 10.06.21 16:46, Martin Liška wrote: Note that flake8 has "plugins". At openSUSE, I install: ... None of those are available on Ubuntu – I probably should nag doko or start using my private computer for the tests ... I support the patch, please install it (except the not intentional change in gcc/c/c-parser.c). Then, please ask Jonathan for updating of the server hook. Cheers, Martin Updated as suggested and with you flake8-fix patch applied on top. Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On Thu, 10 Jun 2021 at 15:08, Tobias Burnus wrote:
>
> (Moved to gcc-patches, missed this when I replied to the initial email)
>
> Regarding patch at: https://gcc.gnu.org/pipermail/gcc/2021-June/236357.html
>
> On 10.06.21 14:45, Jonathan Wakely wrote:
>
> > As well as the "contrig" typo that Florian noticed, the subject says
> > "in in" which should be "is in". And it should be CC'd to gcc-patches.
> >
> > I like this more than my attempt, however ...
> >> --- a/contrib/gcc-changelog/git_repository.py
> >> +++ b/contrib/gcc-changelog/git_repository.py
> >> @@ -59,8 +59,9 @@ def parse_git_revisions(repo_path, revisions,
> >> ref_name=None):
> >>
> >> date = datetime.utcfromtimestamp(c.committed_date)
> >> author = '%s <%s>' % (c.author.name, c.author.email)
> >> -git_info = GitInfo(c.hexsha, date, author,
> >> - c.message.split('\n'), modified_files)
> >> +message = c.message.split('\n')
> >> +git_info = GitInfo(c.hexsha, date, author, message[0],
> >> + message[1:], modified_files)
> > Doesn't using message[1:] here mean that other checks which currently
> > look at all of self.info.lines will no longer check the subject line?
> ...
> > For example, we have:
> > # Skip Update copyright years commits
> > if self.info.lines and self.info.lines[0] == 'Update copyright
> > years.':
> > return
> > This will never match now, because you've extracted that into the
> > 'subject' instead.
>
> Well, it never matched before for git_email.py, it only did match for
> git_repository.py. I think the difference between your work and mine was
> that I started with git_email.py – and you started with git_repository.py.
Yes, because my interest was in making the git gcc-verify alias and
the server hook do the checks, and that works using GitCommit not
GitEmail.
>
> I now pass again the whole message to git_commit.py – also for emails. I
> think that's more consistent when checking for an empty line as second line.
>
> And for the copyright case, I added a testcase :-)
Even better! :-)
> > Aside: We should also have a check that the second line is blank, i.e.
> > the commit message is a single line subject, followed by blank,
> > followed by the body. And if we enforced that, then message[2:] would
> > be better than message[1:].
> Added as check – but I pass now all (also subject + '\n' + body) for the
> email, which I think it easier to grasp.
Nice, thanks.
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 10.06.21 16:46, Martin Liška wrote:
Note that flake8 has "plugins". At openSUSE, I install:
... None of those are available on Ubuntu – I probably should nag doko
or start using my private computer for the tests ...
Updated as suggested and with you flake8-fix patch applied on top.
Tobias
-
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
Thürauf
contrib/gcc-changelog/git_commit.py| 32 --
contrib/gcc-changelog/git_email.py | 22 +++--
contrib/gcc-changelog/test_email.py| 13
contrib/gcc-changelog/test_patches.txt | 60 +-
gcc/c/c-parser.c | 4 +--
5 files changed, 123 insertions(+), 8 deletions(-)
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index bd8c1ff7af2..6f33ad9b420 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -156,7 +156,9 @@ author_line_regex = \
re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.* <.*>)')
additional_author_regex = re.compile(r'^\t(?P\ *)?(?P.* <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
-pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?([0-9]+)$')
+subject_pr_regex = re.compile(r'(^|\W)PR\s+(?P[a-zA-Z+-]+)/(?P\d{4,7})')
+subject_pr2_regex = re.compile(r'[(\[]PR\s*(?P\d{4,7})[)\]]')
+pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?(?P[0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
star_prefix_regex = re.compile(r'\t\*(?P\ *)(?P.*)')
end_of_location_regex = re.compile(r'[\[<(:]')
@@ -298,6 +300,7 @@ class GitCommit:
self.top_level_authors = []
self.co_authors = []
self.top_level_prs = []
+self.subject_prs = set()
self.cherry_pick_commit = None
self.revert_commit = None
self.commit_to_info_hook = commit_to_info_hook
@@ -307,6 +310,10 @@ class GitCommit:
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
+if self.info.lines and len(self.info.lines) > 1 and self.info.lines[1]:
+self.errors.append(Error('Expected empty second line in commit '
+ 'message', info.lines[0]))
+
# Identify first if the commit is a Revert commit
for line in self.info.lines:
m = revert_regex.match(line)
@@ -316,6 +323,19 @@ class GitCommit:
if self.revert_commit:
self.info = self.commit_to_info_hook(self.revert_commit)
+# The following happens for get_email.py:
+if not self.info:
+return
+
+# Extract PR numbers form the subject line
+# Match either [PR] / (PR) or PR component/
+if self.info.lines and not self.revert_commit:
+self.subject_prs = {m.group('pr') for m in subject_pr2_regex.finditer(info.lines[0])}
+for m in subject_pr_regex.finditer(info.lines[0]):
+if not m.group('component') in bug_components:
+self.errors.append(Error('invalid PR component in subject', info.lines[0]))
+self.subject_prs.add(m.group('pr'))
+
# Allow complete deletion of ChangeLog files in a commit
project_files = [f for f in self.info.modified_files
if (self.is_changelog_filename(f[0], allow_suffix=True) and f[1] != 'D')
@@ -346,6 +366,10 @@ class GitCommit:
if not self.errors:
self.check_mentioned_files()
self.check_for_correct_changelog()
+if self.subject_prs:
+self.errors.append(Error('PR %s in subject but not in changelog' %
+ ', '.join(self.subject_prs),
+ self.info.lines[0]))
@property
def success(self):
@@ -460,7 +484,9 @@ class GitCommit:
else:
author_tuple = (m.group('name'), None)
elif pr_regex.match(line):
-component = pr_regex.match(line).group('component')
+m = pr_regex.match(line)
+component = m.group('component')
+pr = m.group('pr')
if not component:
self.errors.append(Error('missing PR component', line))
continue
@@ -469,6 +495,8 @@ class GitCommit:
continue
else:
pr_line = line.lstrip()
+if pr in self.subject_prs:
+self.subject_prs.remove(pr)
elif dr_regex.match(line):
pr_line = line.lstrip()
diff --git a/contrib/gcc-changelog/git_email.py b/contrib/gcc-changelog/git_email.py
index fa62e3ad2f7..87b419cae5d 100755
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 6/10/21 4:42 PM, Tobias Burnus wrote: On 10.06.21 16:24, Martin Liška wrote: I'm sending a small update that handles some flake8 issues and as defined in setup.cfg, line limit for the file is 120 characters. Aha, the known issue that Ubuntu 20.04 has a too old flake8 such that I only see a subset of the errors ... Note that flake8 has "plugins". At openSUSE, I install: python3-flake8 python3-flake8-builtins python3-flake8-bugbear python3-flake8-import-order python3-flake8-quotes python3-flake8-comprehensions I have one question: Do we really need the revert regex? What about using GitCommit::revert_commit? I think it would be fine – glancing at recent commits, I think only the following would be rejected: Good, I would use it then. Martin * https://gcc.gnu.org/g:0886426f5f543e813c1a61e18da6616caf377dfc And one can argue about whether it should or shouldn't be rejected. The other commits I found either have the 'This reverts commit' line – and/or have the PR both in the subject and in the changelog part. Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 10.06.21 16:24, Martin Liška wrote: I'm sending a small update that handles some flake8 issues and as defined in setup.cfg, line limit for the file is 120 characters. Aha, the known issue that Ubuntu 20.04 has a too old flake8 such that I only see a subset of the errors ... I have one question: Do we really need the revert regex? What about using GitCommit::revert_commit? I think it would be fine – glancing at recent commits, I think only the following would be rejected: * https://gcc.gnu.org/g:0886426f5f543e813c1a61e18da6616caf377dfc And one can argue about whether it should or shouldn't be rejected. The other commits I found either have the 'This reverts commit' line – and/or have the PR both in the subject and in the changelog part. Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
On 6/10/21 4:07 PM, Tobias Burnus wrote:
(Moved to gcc-patches, missed this when I replied to the initial email)
Regarding patch at: https://gcc.gnu.org/pipermail/gcc/2021-June/236357.html
On 10.06.21 14:45, Jonathan Wakely wrote:
As well as the "contrig" typo that Florian noticed, the subject says
"in in" which should be "is in". And it should be CC'd to gcc-patches.
I like this more than my attempt, however ...
--- a/contrib/gcc-changelog/git_repository.py
+++ b/contrib/gcc-changelog/git_repository.py
@@ -59,8 +59,9 @@ def parse_git_revisions(repo_path, revisions, ref_name=None):
date = datetime.utcfromtimestamp(c.committed_date)
author = '%s <%s>' % (c.author.name, c.author.email)
- git_info = GitInfo(c.hexsha, date, author,
- c.message.split('\n'), modified_files)
+ message = c.message.split('\n')
+ git_info = GitInfo(c.hexsha, date, author, message[0],
+ message[1:], modified_files)
Doesn't using message[1:] here mean that other checks which currently
look at all of self.info.lines will no longer check the subject line?
...
For example, we have:
# Skip Update copyright years commits
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
This will never match now, because you've extracted that into the
'subject' instead.
Well, it never matched before for git_email.py, it only did match for
git_repository.py. I think the difference between your work and mine was
that I started with git_email.py – and you started with git_repository.py.
I now pass again the whole message to git_commit.py – also for emails. I
think that's more consistent when checking for an empty line as second line.
I see this approach better.
And for the copyright case, I added a testcase :-)
Aside: We should also have a check that the second line is blank, i.e.
the commit message is a single line subject, followed by blank,
followed by the body. And if we enforced that, then message[2:] would
be better than message[1:].
Added as check – but I pass now all (also subject + '\n' + body) for the
email, which I think it easier to grasp. But as three is always an empty
line for emails (to separate header and body), I cannot add a testcase
for it, unfortunately.
Updated patch enclosed.
I'm sending a small update that handles some flake8 issues and as defined in
setup.cfg,
line limit for the file is 120 characters.
I have one question: Do we really need the revert regex? What about using
GitCommit::revert_commit?
Cheers,
Martin
Tobias
-
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
Thürauf
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index 6752ca8..cdafb17968c 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -157,8 +157,7 @@ author_line_regex = \
additional_author_regex = re.compile(r'^\t(?P\ *)?(?P.* <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
subject_pr_skip_regex = re.compile(r'[Rr]evert')
-subject_pr_regex = \
-re.compile(r'(^|\W)PR\s+(?P[a-zA-Z+-]+)/(?P\d{4,7})')
+subject_pr_regex = re.compile(r'(^|\W)PR\s+(?P[a-zA-Z+-]+)/(?P\d{4,7})')
subject_pr2_regex = re.compile(r'[(\[]PR\s*(?P\d{4,7})[)\]]')
pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?(?P[0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
@@ -312,20 +311,16 @@ class GitCommit:
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
-if len(self.info.lines) > 1 and self.info.lines[1] != '':
-self.errors.append(Error('Expected empty second line in commit message',
- info.lines[0]))
+if len(self.info.lines) > 1 and self.info.lines[1]:
+self.errors.append(Error('Expected empty second line in commit message', info.lines[0]))
# Extract PR numbers form the subject line
# Match either [PR] / (PR) or PR component/
if self.info.lines and not subject_pr_skip_regex.search(info.lines[0]):
-self.subject_prs = \
- set([m.group('pr')
- for m in subject_pr2_regex.finditer(info.lines[0])])
+self.subject_prs = {m.group('pr') for m in subject_pr2_regex.finditer(info.lines[0])}
for m in subject_pr_regex.finditer(info.lines[0]):
if not m.group('component') in bug_components:
-self.errors.append(Error('invalid PR component in subject',
- info.lines[0]))
+self.errors.append(Error('invalid PR component in subject', info.lines[0]))
self.subject_pr
Re: [Patch] contrib/gcc-changelog: Check that PR in subject in in changelog
(Moved to gcc-patches, missed this when I replied to the initial email)
Regarding patch at: https://gcc.gnu.org/pipermail/gcc/2021-June/236357.html
On 10.06.21 14:45, Jonathan Wakely wrote:
As well as the "contrig" typo that Florian noticed, the subject says
"in in" which should be "is in". And it should be CC'd to gcc-patches.
I like this more than my attempt, however ...
--- a/contrib/gcc-changelog/git_repository.py
+++ b/contrib/gcc-changelog/git_repository.py
@@ -59,8 +59,9 @@ def parse_git_revisions(repo_path, revisions, ref_name=None):
date = datetime.utcfromtimestamp(c.committed_date)
author = '%s <%s>' % (c.author.name, c.author.email)
-git_info = GitInfo(c.hexsha, date, author,
- c.message.split('\n'), modified_files)
+message = c.message.split('\n')
+git_info = GitInfo(c.hexsha, date, author, message[0],
+ message[1:], modified_files)
Doesn't using message[1:] here mean that other checks which currently
look at all of self.info.lines will no longer check the subject line?
...
For example, we have:
# Skip Update copyright years commits
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
This will never match now, because you've extracted that into the
'subject' instead.
Well, it never matched before for git_email.py, it only did match for
git_repository.py. I think the difference between your work and mine was
that I started with git_email.py – and you started with git_repository.py.
I now pass again the whole message to git_commit.py – also for emails. I
think that's more consistent when checking for an empty line as second line.
And for the copyright case, I added a testcase :-)
Aside: We should also have a check that the second line is blank, i.e.
the commit message is a single line subject, followed by blank,
followed by the body. And if we enforced that, then message[2:] would
be better than message[1:].
Added as check – but I pass now all (also subject + '\n' + body) for the
email, which I think it easier to grasp. But as three is always an empty
line for emails (to separate header and body), I cannot add a testcase
for it, unfortunately.
Updated patch enclosed.
Tobias
-
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
Thürauf
contrib/gcc-changelog: Check that PR in subject is in changelog
This patch checks that a '[PR]' and '(PR)' also appears as PR in the
changelog part of the commit message. And it does likewise for 'PR comp/'
except that then also the component name is checked. (Note that the reverse
is permitted, i.e. PR(s) only appearing in the changelog.)
To avoid false positives, PR numbers in the subject line are ignored,
if 'revert' appears.
Additionally, reject commits with a nonempty second line.
contrib/ChangeLog:
* gcc-changelog/git_commit.py (pr_regex): Add ?P for group('pr').
(subject_pr_skip_regex, subject_pr_regex, subject_pr2_regex): New.
(GitInfo.__init__, GitCommit.parse_changelog): Check subject PRs.
* gcc-changelog/git_email.py (SUBJECT_PREFIX, subject_patch_regex): New.
(GitEmail.__init__): Parse 'Subject:' and pass it to GitInfo.
* gcc-changelog/test_email.py (test_pr_only_in_subject,
test_wrong_pr_comp_in_subject, test_copyright_years): New.
* gcc-changelog/test_patches.txt (0030-PR-c-92746, pr-check1.patch):
Update to avoid triggering the new check.
(0001-rs6000-Support-doubleword, pr-wrong-comp.patch,
copyright-years.patch): New.
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index bd8c1ff7af2..6752ca8 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -156,7 +156,11 @@ author_line_regex = \
re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.* <.*>)')
additional_author_regex = re.compile(r'^\t(?P\ *)?(?P.* <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
-pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?([0-9]+)$')
+subject_pr_skip_regex = re.compile(r'[Rr]evert')
+subject_pr_regex = \
+re.compile(r'(^|\W)PR\s+(?P[a-zA-Z+-]+)/(?P\d{4,7})')
+subject_pr2_regex = re.compile(r'[(\[]PR\s*(?P\d{4,7})[)\]]')
+pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?(?P[0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
star_prefix_regex = re.compile(r'\t\*(?P\ *)(?P.*)')
end_of_location_regex = re.compile(r'[\[<(:]')
@@ -298,6 +302,7 @@ class GitCommit:
self.top_level_authors = []
self.co_authors = []
self.top_level_prs = []
+self.subject_prs = set()
self.cherry_pick_commit = None
self.revert_commit = None
self.commit_to_info_hook = commit_to_info_hook
@@ -307,6 +312,22 @@ class GitCommit:
i
[no subject]
This is the updated patch.
[no subject]
At the moment, for a match_dup in a define_cond_exec, you'd have to give the number in the resulting pattern(s) rather than in the substitute pattern. That's not only wrong, but can also be impossible when the pattern should apply to multiple patterns with different operand numbers. The attached patch fixes this. Bootstrapped on x86_64-pc-linux-gnu. 2020-12-12 Joern Rennecke Fix match_dup bug of define_cond_exec. * gensupport.c (alter_predicate_for_insn): Handle MATCH_DUP. diff --git a/gcc/gensupport.c b/gcc/gensupport.c index e1ca06dbc1e..92275358078 100644 --- a/gcc/gensupport.c +++ b/gcc/gensupport.c @@ -1230,6 +1230,7 @@ alter_predicate_for_insn (rtx pattern, int alt, int max_op, case MATCH_OPERATOR: case MATCH_SCRATCH: case MATCH_PARALLEL: +case MATCH_DUP: XINT (pattern, 0) += max_op; break;
[no subject]
>From b1774ab1c8aad82b7a5d975ef90c6d3f633780ee Mon Sep 17 00:00:00 2001
From: expnkx
Date: Wed, 14 Apr 2021 03:14:28 -0400
Subject: [PATCH] Fix intrinsics mm_malloc.h in freestanding [PR100057]
C does not have stdlib.h and C++ cstdint in freestanding does not malloc
either. This leads
to fail of compilation even with -ffrestanding flag
Only gmm_malloc checks errno, everything else does not. So we remove the errno
in gmm_malloc too. There is no reason freestanding should behave differently
with hosted.
gcc/ChangeLog
PR/100057:
gcc/config/i386/gmm_malloc.h: use __builtin_malloc and __builtin_free
instead
gcc/config/i386/pmm_malloc.h: use __builtin_malloc and __builtin_free
instead
gcc/config/rs6000/mm_malloc.h: use __builtin_malloc and __builtin_free
instead
---
gcc/config/i386/gmm_malloc.h | 13 -
gcc/config/i386/pmm_malloc.h | 13 +
gcc/config/rs6000/mm_malloc.h | 13 +
3 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/gcc/config/i386/gmm_malloc.h b/gcc/config/i386/gmm_malloc.h
index 70b38ab557b..276a5f50023 100644
--- a/gcc/config/i386/gmm_malloc.h
+++ b/gcc/config/i386/gmm_malloc.h
@@ -24,10 +24,7 @@
#ifndef _MM_MALLOC_H_INCLUDED
#define _MM_MALLOC_H_INCLUDED
-#include
-#if __STDC_HOSTED__
-#include
-#endif
+#include
static __inline__ void *
_mm_malloc (size_t __size, size_t __align)
@@ -38,9 +35,6 @@ _mm_malloc (size_t __size, size_t __align)
/* Error if align is not a power of two. */
if (__align & (__align - 1))
{
-#if __STDC_HOSTED__
- errno = EINVAL;
-#endif
return ((void *) 0);
}
@@ -54,7 +48,7 @@ _mm_malloc (size_t __size, size_t __align)
if (__align < 2 * sizeof (void *))
__align = 2 * sizeof (void *);
- __malloc_ptr = malloc (__size + __align);
+ __malloc_ptr = __builtin_malloc (__size + __align);
if (!__malloc_ptr)
return ((void *) 0);
@@ -72,7 +66,8 @@ static __inline__ void
_mm_free (void *__aligned_ptr)
{
if (__aligned_ptr)
-free (((void **) __aligned_ptr)[-1]);
+__builtin_free (((void **) __aligned_ptr)[-1]);
}
+
#endif /* _MM_MALLOC_H_INCLUDED */
diff --git a/gcc/config/i386/pmm_malloc.h b/gcc/config/i386/pmm_malloc.h
index 1b0bfe37852..3b97107ccfc 100644
--- a/gcc/config/i386/pmm_malloc.h
+++ b/gcc/config/i386/pmm_malloc.h
@@ -24,14 +24,19 @@
#ifndef _MM_MALLOC_H_INCLUDED
#define _MM_MALLOC_H_INCLUDED
-#include
+#include
/* We can't depend on since the prototype of posix_memalign
may not be visible. */
#ifndef __cplusplus
extern int posix_memalign (void **, size_t, size_t);
#else
-extern "C" int posix_memalign (void **, size_t, size_t) throw ();
+extern "C" int posix_memalign (void **, size_t, size_t)
+#if __cplusplus >= 201103L
+noexcept;
+#else
+throw ();
+#endif
#endif
static __inline void *
@@ -39,7 +44,7 @@ _mm_malloc (size_t __size, size_t __alignment)
{
void *__ptr;
if (__alignment == 1)
-return malloc (__size);
+return __builtin_malloc (__size);
if (__alignment == 2 || (sizeof (void *) == 8 && __alignment == 4))
__alignment = sizeof (void *);
if (posix_memalign (&__ptr, __alignment, __size) == 0)
@@ -51,7 +56,7 @@ _mm_malloc (size_t __size, size_t __alignment)
static __inline void
_mm_free (void *__ptr)
{
- free (__ptr);
+ __builtin_free (__ptr);
}
#endif /* _MM_MALLOC_H_INCLUDED */
diff --git a/gcc/config/rs6000/mm_malloc.h b/gcc/config/rs6000/mm_malloc.h
index c04348068e0..82aaab411da 100644
--- a/gcc/config/rs6000/mm_malloc.h
+++ b/gcc/config/rs6000/mm_malloc.h
@@ -24,14 +24,19 @@
#ifndef _MM_MALLOC_H_INCLUDED
#define _MM_MALLOC_H_INCLUDED
-#include
+#include
/* We can't depend on since the prototype of posix_memalign
may not be visible. */
#ifndef __cplusplus
extern int posix_memalign (void **, size_t, size_t);
#else
-extern "C" int posix_memalign (void **, size_t, size_t) throw ();
+extern "C" int posix_memalign (void **, size_t, size_t)
+#if __cplusplus >= 201103L
+noexcept;
+#else
+throw ();
+#endif
#endif
static __inline void *
@@ -44,7 +49,7 @@ _mm_malloc (size_t size, size_t alignment)
void *ptr;
if (alignment == malloc_align && alignment == vec_align)
-return malloc (size);
+return __builtin_malloc (size);
if (alignment < vec_align)
alignment = vec_align;
if (posix_memalign (, alignment, size) == 0)
@@ -56,7 +61,7 @@ _mm_malloc (size_t size, size_t alignment)
static __inline void
_mm_free (void * ptr)
{
- free (ptr);
+ __builtin_free (ptr);
}
#endif /* _MM_MALLOC_H_INCLUDED */
--
2.25.1
Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10
[no subject]
This libgo patch by Clément Chigot correctly handles AIX FAT library
creation. The previous patch wasn't working everytime. Especially
when AR had "-X32_64", the new .so would replace the default one and
not just being added. Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu. Committed to mainline.
Ian
de93b8d5bfb3d2652e5e166f3e4db1c25a3a2c57
diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index 08daa1a5924..e443282d0e8 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-e08f1d7d1bc14c0a29eb9ee17980f14fa2397239
+fe5d94c5792f7f990004c3dee0ea501835512200
The first line of this file holds the git revision number of the last
merge done from the gofrontend repository.
diff --git a/libgo/Makefile.am b/libgo/Makefile.am
index 88ea2728bc3..1112ee27df6 100644
--- a/libgo/Makefile.am
+++ b/libgo/Makefile.am
@@ -1244,8 +1244,15 @@ endif
all-local: $(ALL_LOCAL_DEPS)
MAJOR=$(firstword $(subst :, ,$(libtool_VERSION)))
+
+# If we want to use "AR -r" when creating AIX FAT archives,
+# AR must be stripped of all its -X flags.
+# Otherwize, if AR was defined with -X32_64, the replace option would
+# erase the default .so when adding the extra one. There is no
+# order priority within -X flags.
add-aix-fat-library: all-multi
@if test "$(MULTIBUILDTOP)" = ""; then \
- ${AR} -X$(AIX_DEFAULT_ARCH) rc .libs/$(PACKAGE).a
../ppc$(AIX_DEFAULT_ARCH)/$(PACKAGE)/.libs/$(PACKAGE).so.$(MAJOR); \
- ${AR} -X$(AIX_DEFAULT_ARCH) rc
../pthread/$(PACKAGE)/.libs/$(PACKAGE).a
../pthread/ppc$(AIX_DEFAULT_ARCH)/$(PACKAGE)/.libs/$(PACKAGE).so.$(MAJOR); \
+ arx=`echo $(AR) | sed -e 's/-X[^ ]*//g'`; \
+ $${arx} -X$(AIX_EXTRA_ARCH) rc .libs/$(PACKAGE).a
../ppc$(AIX_EXTRA_ARCH)/$(PACKAGE)/.libs/$(PACKAGE).so.$(MAJOR); \
+ $${arx} -X$(AIX_EXTRA_ARCH) rc
../pthread/$(PACKAGE)/.libs/$(PACKAGE).a
../pthread/ppc$(AIX_EXTRA_ARCH)/$(PACKAGE)/.libs/$(PACKAGE).so.$(MAJOR); \
fi
diff --git a/libgo/configure.ac b/libgo/configure.ac
index db5848e36ad..abc58b87b53 100644
--- a/libgo/configure.ac
+++ b/libgo/configure.ac
@@ -38,12 +38,12 @@ case ${host} in
GOCFLAGS="$GOCFLAGS -fno-section-anchors"
# Check default architecture for FAT library creation
-if test -z "`$(CC) -x c -E /dev/null -g3 -o - | grep 64BIT`" ; then
-AIX_DEFAULT_ARCH='64'
+if test -z "`$CC -x c -E /dev/null -g3 -o - | grep 64BIT`" ; then
+AIX_EXTRA_ARCH='64'
else
-AIX_DEFAULT_ARCH='32'
+AIX_EXTRA_ARCH='32'
fi
-AC_SUBST(AIX_DEFAULT_ARCH)
+AC_SUBST(AIX_EXTRA_ARCH)
;;
esac
[no subject]
>> > We've moved more and more to stronly-typed data structures
>> > so I'd not like to see 'auto' everywhere - it should be still
>> > obvious what kind of objects we're working with where they
>> > matter. IMHO they do not matter for example for iterators.
>> > I don't care about the iterator type but about the type of
>> > the object and the container.
>>
>> Also agreed. :-) How about this as a starting point:
>>
>> ---
>> Use auto for:
>>
>> - the result of casts or other expressions that give the type
>> explicitly. E.g.:
>>
>> if (auto *table = dyn_cast (insn))
>>
>> instead of:
>>
>> if (rtx_jump_table_data *table = dyn_cast (insn))
>>
>> - iterator types. E.g.:
>>
>> auto it = foo.begin ();
>>
>> instead of:
>>
>> foo_type::iterator it = foo.begin ();
>>
>> - expressions that provide an alternative view of something,
>> when the expression is bound to a read-only temporary. E.g.:
>>
>> auto val1 = wi::to_wide (...);
>> auto val2 = wi::uhwi (12, 16);
>>
>> instead of:
>>
>> wide_int val1 = wi::to_wide (...);
>> wide_int val2 = wi::uhwi (12, 16);
>>
>> (Using "wide_int" is less efficient than using the natural type of
>> the expression.)
>>
>> - the type of a lambda expression. E.g.:
>>
>> auto f = [] (int x) { return x + 1; };
>
> Those are all good examples. Mind putting that into a patch
> for the coding conventions?
How's this? I added "new" expressions as another example of the
first category.
I'm sure I've missed other good uses, but we can always add to the
list later if necessary.
Thanks,
Richard
>From 10b27e367de0fa9d5bf91544385401cdcbdb8c00 Mon Sep 17 00:00:00 2001
From: Richard Sandiford
Date: Fri, 15 May 2020 14:58:46 +0100
Subject: [PATCH] Describe coding conventions surrounding "auto"
---
htdocs/codingconventions.html | 53 +++
htdocs/codingrationale.html | 17 +++
2 files changed, 70 insertions(+)
diff --git a/htdocs/codingconventions.html b/htdocs/codingconventions.html
index f4732ef6..ae49fb91 100644
--- a/htdocs/codingconventions.html
+++ b/htdocs/codingconventions.html
@@ -51,6 +51,7 @@ the conventions separately from any other changes to the
code.
Language Use
Variable Definitions
+Use of auto
Struct Definitions
Class Definitions
Constructors and Destructors
@@ -884,6 +885,58 @@ Variables may be simultaneously defined and tested in
control expressions.
Rationale and Discussion
+Use of auto
+
+auto should be used in the following circumstances:
+
+ when the expression gives the C++ type explicitly. For example
+
+
+if (auto *table = dyn_cast rtx_jump_table_data *
(insn)) // OK
+ ...
+if (rtx_jump_table_data *table = dyn_cast rtx_jump_table_data *
(insn)) // Avoid
+ ...
+auto *map = new hash_map tree, size_t;
// OK
+hash_map tree, size_t *map = new hash_map tree, size_t; //
Avoid
+
+This rule does not apply to abbreviated type names embedded in
+an identifier, such as the result of tree_to_shwi.
+
+
+when the expression simply provides an alternative view of an object
+and is bound to a read-only temporary. For example:
+
+
+auto wioff = wi::to_wide (off); // OK
+wide_int wioff = wi::to_wide (off); // Avoid if wioff is read-only
+auto minus1 = std::shwi (-1, prec); // OK
+wide_int minus1 = std::shwi (-1, prec); // Avoid if minus1 is
read-only
+
+In principle this rule applies to other views of an object too,
+such as a reversed view of a list, or a sequential view of a
+hash_set. It does not apply to general temporaries.
+
+
+the type of an iterator. For example:
+
+
+auto it = std::find (names.begin (), names.end (), needle);
// OK
+vector name_map::iterator it = std::find (names.begin (),
+names.end (), needle); //
Avoid
+
+
+the type of a lambda expression. For example:
+
+
+auto f = [] (int x) { return x + 1; }; //
OK
+
+
+
+auto should not be used in other contexts.
+
+
+Rationale and Discussion
+
Struct Definitions
diff --git a/htdocs/codingrationale.html b/htdocs/codingrationale.html
index 0b44f1da..a919023c 100644
--- a/htdocs/codingrationale.html
+++ b/htdocs/codingrationale.html
@@ -50,6 +50,23 @@ if (info *q = get_any_available_info ()) {
}
+Use of auto
+
+The reason for preferring auto in expressions like:
+auto wioff = wi::to_wide (off);
+is that using the natural type of the expression is more efficient than
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 02/03/2020 14:41, Jonathan Wakely wrote: On Mon, 2 Mar 2020 at 14:31, Nathan Sidwell wrote: On 3/2/20 8:01 AM, Richard Earnshaw (lists) wrote: On 27/02/2020 13:37, Nathan Sidwell wrote: On 2/3/20 6:41 AM, Richard Earnshaw (lists) wrote: On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: [updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). I've not seen any follow-up to this version. Should we go ahead and adopt this? I'd like to. But have we reached consensus? Seems that every time I produce a revised version of the text we end up in another round of bike shedding. (Is that a word?) I'm not sure I've seen a specific proposal following yours. Some suggestions for differences, with varying degrees of forcefulness. I still say go for it. Go for it. It's not like we're going to take away commit privs from people who use slight variations on the scheme. It's better to have a written policy that people should aim towards, and most people will follow in most cases. OK, pushed. Folk can, of course, now propose changes to the text as it stands... R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 2 Mar 2020 at 14:31, Nathan Sidwell wrote: > > On 3/2/20 8:01 AM, Richard Earnshaw (lists) wrote: > > On 27/02/2020 13:37, Nathan Sidwell wrote: > >> On 2/3/20 6:41 AM, Richard Earnshaw (lists) wrote: > >>> On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: > >>>> > >>>> [updated based on v2 discussions] > >>>> > >>>> This patch proposes some new (additional) rules for email subject lines > >>>> when contributing to GCC. The goal is to make sure that, as far as > >>>> possible, the subject for a patch will form a good summary when the > >>>> message is committed to the repository if applied with 'git am'. Where > >>>> possible, I've tried to align these rules with those already in > >>>> use for glibc, so that the differences are minimal and only where > >>>> necessary. > >>>> > >>>> Some things that differ from existing practice (at least by some > >>>> people) > >>>> are: > >>>> > >>>> - Use ':' rather than '[]' > >>>>- This is more git friendly and works with 'git am'. > >>>> - Put bug numbers at the end of the line rather than the beginning. > >>>>- The bug number is useful, but not as useful as the brief summary. > >>>> Also, use the shortened form, as the topic part is more usefully > >>>> conveyed in the proper topic field (see above). > >>> > >>> I've not seen any follow-up to this version. Should we go ahead and > >>> adopt this? > > > I'd like to. But have we reached consensus? Seems that every time I > > produce a revised version of the text we end up in another round of bike > > shedding. (Is that a word?) > > I'm not sure I've seen a specific proposal following yours. Some > suggestions for differences, with varying degrees of forcefulness. I > still say go for it. Go for it. It's not like we're going to take away commit privs from people who use slight variations on the scheme. It's better to have a written policy that people should aim towards, and most people will follow in most cases.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 3/2/20 8:01 AM, Richard Earnshaw (lists) wrote: On 27/02/2020 13:37, Nathan Sidwell wrote: On 2/3/20 6:41 AM, Richard Earnshaw (lists) wrote: On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: [updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). I've not seen any follow-up to this version. Should we go ahead and adopt this? I'd like to. But have we reached consensus? Seems that every time I produce a revised version of the text we end up in another round of bike shedding. (Is that a word?) I'm not sure I've seen a specific proposal following yours. Some suggestions for differences, with varying degrees of forcefulness. I still say go for it. nathan -- Nathan Sidwell
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Mar 02, 2020 at 01:01:46PM +, Richard Earnshaw (lists) wrote: > I'd like to. But have we reached consensus? Seems that every time I > produce a revised version of the text we end up in another round of bike > shedding. (Is that a word?) It's called a "lap", but "criterium" would be more suitable here. Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 27/02/2020 13:37, Nathan Sidwell wrote: On 2/3/20 6:41 AM, Richard Earnshaw (lists) wrote: On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: [updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). I've not seen any follow-up to this version. Should we go ahead and adopt this? do it! do it! do it! do it! nathan :-) I'd like to. But have we reached consensus? Seems that every time I produce a revised version of the text we end up in another round of bike shedding. (Is that a word?) R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 2/3/20 6:41 AM, Richard Earnshaw (lists) wrote: On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: [updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). I've not seen any follow-up to this version. Should we go ahead and adopt this? do it! do it! do it! do it! nathan -- Nathan Sidwell
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 18:09, Michael Matz wrote: But suggesting that using the subject line for tagging is recommended can lead to subjects like [PATCH][GCC][Foo][component] Fix foo component bootstrap failure in an e-mail directed to gcc-patches@gcc.gnu.org (from somewhen last year, where Foo/foo was an architecture; I'm really not trying to single out the author). That is, _none_ of the four tags carried any informational content. I partially disagree with this. Certainly there's pointless redundancy it this example, but I'd rather have the tags with a meaningful subject than a generic subject with no tags. gcc-patches is a high-volume list in which most of the content is outside my range of interest and/or understanding. If I stay on top of it then I can read all the subject lines, at least, and probably select a few threads to learn about something new, but if I let the list get away from me for even a short while then it's too much to handle. I do have filters set up to highlight subjects for which I should pay attention and if people are in the habit of tagging subjects then that becomes much more reliable. Conversely, the tags help me quickly decide what not to read. I see that some people are using a "[email tag] git tag: msg" format, and I quite like that. Andrew
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 06:20:20PM +, Michael Matz wrote: > On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > > Well, I'd review a patch differently depending on whether or not it was > > already committed, a patch requiring review or an RFC looking for more > > general comments, so I *do* think such an email prefix is useful. > > As I said: a very good argument must be made; it might be that rfc falls > into the useful-tag category. Yes, "rfc" can be useful to know *before* reading the mail. > > The 50 char limit seems to come from wanting git log --oneline to not wrap > > in > > an 80 column terminal. Whilst laudable, I'm not sure that such a limit > > doesn't become too restrictive and then lead to hard-to-understand > > summaries. > > In my experience hard-to-understand summaries are more related to people > writing them than to length, IOW, I fear a larger limit like 72 characters > won't help that. Yup. If it helps, don't think of it as "summary", think of it as "title". Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 06:54:05PM +0100, Jakub Jelinek wrote: > On Mon, Feb 03, 2020 at 05:48:57PM +, Michael Matz wrote: > > On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > > > > > The idea is that the [...] part is NOT part of the commit, only part of > > > the email. > > > > I understand that, but the subject line of this thread says "e-mail > > subject lines", so I thought we were talking about, well, exactly that; > > and I see no value of these tags in e-mails either. > > In email, they do carry information that is useful there, the distinction > whether a patch has been committed already and doesn't need review from > others, or whether it is a patch intended for patch review, or just a RFC > patch that is not yet ready for review, but submitter is looking for some > feedback. It's irrelevant whether a patch is committed or not whather it needs review, imnsho :-) "rfc" is useful, certainly. It makes clear that the sender would like some help, and/or that the subject might be controversial, both things that have more time pressure than most. Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
Hello, On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > Well, I'd review a patch differently depending on whether or not it was > already committed, a patch requiring review or an RFC looking for more > general comments, so I *do* think such an email prefix is useful. As I said: a very good argument must be made; it might be that rfc falls into the useful-tag category. > >> 'git am' would strip leading [...] automatically unless > >> you've configured, or asked git to do otherwise. So that leading part > >> is not counted for the length calculation. > > > > There's still e-mail netiquette which also should be obeyed, or at least > > not contradicted by git netiquette. > > The 50 char limit seems to come from wanting git log --oneline to not wrap in > an 80 column terminal. Whilst laudable, I'm not sure that such a limit > doesn't become too restrictive and then lead to hard-to-understand summaries. In my experience hard-to-understand summaries are more related to people writing them than to length, IOW, I fear a larger limit like 72 characters won't help that. And, as Segher put it, we aren't really talking about limits, only about suggestions, if you _really_ have to mention that 40-character function name in which you fixed something in your subject, then yeah, you'll go over the 50 chars. But as recommendation the 50 chars make more sense than the 72 chars, IMHO. Ciao, Michael.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
Hello, On Mon, 3 Feb 2020, Jakub Jelinek wrote: > > > The idea is that the [...] part is NOT part of the commit, only part of > > > the email. > > > > I understand that, but the subject line of this thread says "e-mail > > subject lines", so I thought we were talking about, well, exactly that; > > and I see no value of these tags in e-mails either. > > In email, they do carry information that is useful there, the distinction > whether a patch has been committed already and doesn't need review from > others, or whether it is a patch intended for patch review, or just a RFC > patch that is not yet ready for review, but submitter is looking for some > feedback. For tags like [cmt] or [rfc] I don't have much gripe, though I do think that info could be given in the body, and that e.g. in e-mail archives (where the tags are not removed automatically) they carry the same value as in git log, namely zero. But suggesting that using the subject line for tagging is recommended can lead to subjects like [PATCH][GCC][Foo][component] Fix foo component bootstrap failure in an e-mail directed to gcc-patches@gcc.gnu.org (from somewhen last year, where Foo/foo was an architecture; I'm really not trying to single out the author). That is, _none_ of the four tags carried any informational content. Ciao, Michael.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 3 Feb 2020, Michael Matz wrote: > I understand that, but the subject line of this thread says "e-mail > subject lines", so I thought we were talking about, well, exactly that; > and I see no value of these tags in e-mails either. I agree that [PATCH] is not useful (and in general, anything in the subject line before the actual meaningful commit summary should be as short as possible). [RFC] might be useful, but not [PATCH]. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 17:48, Michael Matz wrote: Hi, On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: The idea is that the [...] part is NOT part of the commit, only part of the email. I understand that, but the subject line of this thread says "e-mail subject lines", so I thought we were talking about, well, exactly that; and I see no value of these tags in e-mails either. (They might have a low but non-zero value for projects that use a single mailing list for patches and generic discussion, but we are not such project) Basically: if they are deemed to clutter the git log for whatever reason, then there must be a very good argument for why they not also clutter e-mail subject lines, but instead are essential to have there, but not in the log. Well, I'd review a patch differently depending on whether or not it was already committed, a patch requiring review or an RFC looking for more general comments, so I *do* think such an email prefix is useful. 'git am' would strip leading [...] automatically unless you've configured, or asked git to do otherwise. So that leading part is not counted for the length calculation. There's still e-mail netiquette which also should be obeyed, or at least not contradicted by git netiquette. The 50 char limit seems to come from wanting git log --oneline to not wrap in an 80 column terminal. Whilst laudable, I'm not sure that such a limit doesn't become too restrictive and then lead to hard-to-understand summaries. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 05:48:57PM +, Michael Matz wrote: > On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > > > The idea is that the [...] part is NOT part of the commit, only part of > > the email. > > I understand that, but the subject line of this thread says "e-mail > subject lines", so I thought we were talking about, well, exactly that; > and I see no value of these tags in e-mails either. In email, they do carry information that is useful there, the distinction whether a patch has been committed already and doesn't need review from others, or whether it is a patch intended for patch review, or just a RFC patch that is not yet ready for review, but submitter is looking for some feedback. Jakub
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
Hi, On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > The idea is that the [...] part is NOT part of the commit, only part of > the email. I understand that, but the subject line of this thread says "e-mail subject lines", so I thought we were talking about, well, exactly that; and I see no value of these tags in e-mails either. (They might have a low but non-zero value for projects that use a single mailing list for patches and generic discussion, but we are not such project) Basically: if they are deemed to clutter the git log for whatever reason, then there must be a very good argument for why they not also clutter e-mail subject lines, but instead are essential to have there, but not in the log. > 'git am' would strip leading [...] automatically unless > you've configured, or asked git to do otherwise. So that leading part > is not counted for the length calculation. There's still e-mail netiquette which also should be obeyed, or at least not contradicted by git netiquette. Ciao, Michael.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 17:31, Michael Matz wrote: Hello, On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: Where does your '50 chars' limit come from? It's not in the glibc text, and it's not in the linux kernel text either. AFAICT this is your invention and you seem to be the only person proposing it. Nope, it's fairly common, so much so that it's included in the "commonly accepted rules" that googling for "git subject lines" gives you (as a snippet glimpse from some website), and that vim changes color when spilling over 50 chars. I actually thought it was universal and obvious until this thread (which is why I admittedly only did the above google right before writing this mail). For the rationale: 'git log --oneline' with hash and author or date should fit the usual 72 char limit. (An 11-character hash plus space alone would come out as 60 chars for the subject) That's also the reason why some people (certainly me) are nervous about or dislike all the "tags" in the subject line. E.g. what essential information (and subjects are for essential info, right?) is "[committed]" (or, even worse "[patch]") supposed to transport? If the rest of the subject doesn't interest me, I don't care if something was committed or not; if it _does_ interest me, then I'm going to look at the mail/patch either way, if committed or not; at which point the info if the author required review or has already committed it could be gives in the body as well. Similar for some other metainfo tags. (The "subsystem:" is good, though). And if we must have these tags, then why not at least short ones? Why isn't "[cmt]" or something enough? There will be very few tags, so they become mnemonic pretty much immediately. What becomes clearer when writing "[patch v2 1/13]" in comparison to "[v2 1/13]"? The idea is that the [...] part is NOT part of the commit, only part of the email. 'git am' would strip leading [...] automatically unless you've configured, or asked git to do otherwise. So that leading part is not counted for the length calculation. R. Ciao, Michael.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 01:59:58PM +, Richard Earnshaw (lists) wrote:
> I think the linux rule (the whole line, not including the parts that are
> removed on commit, should not exceed 75 characters) is far more sensible
> - which is why this draft states this.
FWIW, on a slightly older kernel tree:
$ git log --oneline --format=%s|awk '{print length}'|sort -n|uniq -c|sort -r
-n|head -10
25059 50
24872 49
24563 47
24547 48
24261 51
24050 52
23773 53
23527 46
23231 54
22907 45
On git.git:
$ git log --oneline --format=%s|awk '{print length}'|sort -n|uniq -c|sort -r
-n|head -10
1694 50
1657 49
1649 44
1623 46
1621 48
1606 43
1579 45
1558 47
1544 51
1499 37
Sometimes longer subject lines are unavoidable. Most of the time they
can be pretty easily avoided, and they hurt usability. It is okay to
spend a few minutes on it, think of the thousands of people who will
later read it.
Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
Hello, On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > Where does your '50 chars' limit come from? It's not in the glibc text, > and it's not in the linux kernel text either. AFAICT this is your > invention and you seem to be the only person proposing it. Nope, it's fairly common, so much so that it's included in the "commonly accepted rules" that googling for "git subject lines" gives you (as a snippet glimpse from some website), and that vim changes color when spilling over 50 chars. I actually thought it was universal and obvious until this thread (which is why I admittedly only did the above google right before writing this mail). For the rationale: 'git log --oneline' with hash and author or date should fit the usual 72 char limit. (An 11-character hash plus space alone would come out as 60 chars for the subject) That's also the reason why some people (certainly me) are nervous about or dislike all the "tags" in the subject line. E.g. what essential information (and subjects are for essential info, right?) is "[committed]" (or, even worse "[patch]") supposed to transport? If the rest of the subject doesn't interest me, I don't care if something was committed or not; if it _does_ interest me, then I'm going to look at the mail/patch either way, if committed or not; at which point the info if the author required review or has already committed it could be gives in the body as well. Similar for some other metainfo tags. (The "subsystem:" is good, though). And if we must have these tags, then why not at least short ones? Why isn't "[cmt]" or something enough? There will be very few tags, so they become mnemonic pretty much immediately. What becomes clearer when writing "[patch v2 1/13]" in comparison to "[v2 1/13]"? Ciao, Michael.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 01:59:58PM +, Richard Earnshaw (lists) wrote: > On 03/02/2020 13:54, Segher Boessenkool wrote: > >None of this are *rules*. We should not pretend they are. An email > >subject should be useful to what the receivers of that email use it for: > >see if it very interesting to them, see if it probably not interesting > >to them: that's the "smth: " at the start, and the PR number at the end, > >and of course the actual subject itself, so we should not put in too > >much fluff in the subject, there needs to be room left (in the less than > >fifty chars total) for an actual subject :-) > > > >(The example in the patch does not capitalise the subject line, btw. > >It should.) > > Where does your '50 chars' limit come from? It's not in the glibc text, > and it's not in the linux kernel text either. AFAICT this is your > invention and you seem to be the only person proposing it. > > I think the linux rule (the whole line, not including the parts that are > removed on commit, should not exceed 75 characters) is far more sensible > - which is why this draft states this. Oh, and it's not a limit, just a strong recommendation. Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 01:59:58PM +, Richard Earnshaw (lists) wrote: > On 03/02/2020 13:54, Segher Boessenkool wrote: > >None of this are *rules*. We should not pretend they are. An email > >subject should be useful to what the receivers of that email use it for: > >see if it very interesting to them, see if it probably not interesting > >to them: that's the "smth: " at the start, and the PR number at the end, > >and of course the actual subject itself, so we should not put in too > >much fluff in the subject, there needs to be room left (in the less than > >fifty chars total) for an actual subject :-) > > > >(The example in the patch does not capitalise the subject line, btw. > >It should.) > > Where does your '50 chars' limit come from? It's not in the glibc text, > and it's not in the linux kernel text either. AFAICT this is your > invention and you seem to be the only person proposing it. Ha, no. It seems it originally is from https://github.com/git/git/commit/927a503cd07718ea0f700052043f383253904a56 and it has been part of "man git commit" since https://github.com/git/git/commit/936f32d3de468db8daf853155ddd5c6b191af60c (2006 and 2007, resp.) Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 3 Feb 2020 15:04:57 + "Richard Earnshaw (lists)" wrote: > On 03/02/2020 14:13, Jonathan Wakely wrote: > > On Mon, 3 Feb 2020 at 14:00, Richard Earnshaw (lists) wrote: > >> Where does your '50 chars' limit come from? It's not in the glibc text, > >> and it's not in the linux kernel text either. AFAICT this is your > >> invention and you seem to be the only person proposing it. > > > > It's a fairly well established convention, e.g. > > https://chris.beams.io/posts/git-commit/ and it's what Github suggests > > (and whines if you go past it). > > > > That suggest it as a limit for everything. If you have a tag and a bug > number then then that would leave very little for the real part of the > summary and would likely lead to something meaningless or > incomprehensible in the remaining characters. That might be OK for > small projects, but for something the size of gcc, I think keeping the > extra flexibility is useful. Hopefully I've been following this discussion properly. It's important to clarify that there are two *limits* in play here. The subject should generally be kept to 50 chars or less, This is because the subject is displayed in the output of commands like 'git log --oneline' and 'git shortlog' which indents more than git log. Then the message body should wrap after 72-75 chars, again as the output of the likes of 'git log' indent the message by 4 chars. Using vim as the git commit message editor, vim will show you when your subject line is over 50 chars and it wraps the message body after 72 chars. Cheers, Andrew
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 15:13, Richard Earnshaw (lists) wrote: On 03/02/2020 14:10, Jason Merrill wrote: On Mon, Feb 3, 2020 at 7:57 AM Alexander Monakov wrote: On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: Upper case is what glibc has, though it appears that it's a rule that is not strictly followed. If we change it, then it becomes another friction point between developer groups. Personally, I'd leave it as is, then turn a blind eye to such minor non-conformance. In that case can we simply say that both 'committed' and 'COMMITTED' are okay, if we know glibc doesn't follow that rule and anticipate we will not follow it either? And perhaps something shorter? "committed" is a long word. [PUSHED]? Jason "Committed" is the accepted term in glibc. If we insist on something different, then that becomes a friction point. "COMMITTED", not "Committed". On the other hand, I really don't care personally as long as the meaning is clear. I'd be happy with the super-set of all of these, since it only appears in email messages, not the final commit. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 14:10, Jason Merrill wrote: On Mon, Feb 3, 2020 at 7:57 AM Alexander Monakov wrote: On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: Upper case is what glibc has, though it appears that it's a rule that is not strictly followed. If we change it, then it becomes another friction point between developer groups. Personally, I'd leave it as is, then turn a blind eye to such minor non-conformance. In that case can we simply say that both 'committed' and 'COMMITTED' are okay, if we know glibc doesn't follow that rule and anticipate we will not follow it either? And perhaps something shorter? "committed" is a long word. [PUSHED]? Jason "Committed" is the accepted term in glibc. If we insist on something different, then that becomes a friction point. On the other hand, I really don't care personally as long as the meaning is clear. I'd be happy with the super-set of all of these, since it only appears in email messages, not the final commit. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 14:13, Jonathan Wakely wrote: On Mon, 3 Feb 2020 at 14:00, Richard Earnshaw (lists) wrote: Where does your '50 chars' limit come from? It's not in the glibc text, and it's not in the linux kernel text either. AFAICT this is your invention and you seem to be the only person proposing it. It's a fairly well established convention, e.g. https://chris.beams.io/posts/git-commit/ and it's what Github suggests (and whines if you go past it). That suggest it as a limit for everything. If you have a tag and a bug number then then that would leave very little for the real part of the summary and would likely lead to something meaningless or incomprehensible in the remaining characters. That might be OK for small projects, but for something the size of gcc, I think keeping the extra flexibility is useful. I think the linux rule (the whole line, not including the parts that are removed on commit, should not exceed 75 characters) is far more sensible - which is why this draft states this. I'm OK with that. OK, thanks. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 3 Feb 2020 at 14:00, Richard Earnshaw (lists) wrote: > Where does your '50 chars' limit come from? It's not in the glibc text, > and it's not in the linux kernel text either. AFAICT this is your > invention and you seem to be the only person proposing it. It's a fairly well established convention, e.g. https://chris.beams.io/posts/git-commit/ and it's what Github suggests (and whines if you go past it). > I think the linux rule (the whole line, not including the parts that are > removed on commit, should not exceed 75 characters) is far more sensible > - which is why this draft states this. I'm OK with that.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 3, 2020 at 7:57 AM Alexander Monakov wrote: > On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > > > Upper case is what glibc has, though it appears that it's a rule that is > not > > strictly followed. If we change it, then it becomes another friction > point > > between developer groups. Personally, I'd leave it as is, then turn a > blind > > eye to such minor non-conformance. > > In that case can we simply say that both 'committed' and 'COMMITTED' are > okay, > if we know glibc doesn't follow that rule and anticipate we will not follow > it either? > And perhaps something shorter? "committed" is a long word. [PUSHED]? Jason
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 13:54, Segher Boessenkool wrote: On Mon, Feb 03, 2020 at 02:54:05PM +0300, Alexander Monakov wrote: On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: I've not seen any follow-up to this version. Should we go ahead and adopt this? Can we please go with 'committed' (lowercase) rather than all-caps COMMITTED? Spelling this with all-caps seems like a recent thing on gcc-patches, before everyone used the lowercase version, which makes more sense (no need to shout about the thing that didn't need any discussion before applying the patch). Lower case certainly makes more sense. None of this are *rules*. We should not pretend they are. An email subject should be useful to what the receivers of that email use it for: see if it very interesting to them, see if it probably not interesting to them: that's the "smth: " at the start, and the PR number at the end, and of course the actual subject itself, so we should not put in too much fluff in the subject, there needs to be room left (in the less than fifty chars total) for an actual subject :-) (The example in the patch does not capitalise the subject line, btw. It should.) Segher Where does your '50 chars' limit come from? It's not in the glibc text, and it's not in the linux kernel text either. AFAICT this is your invention and you seem to be the only person proposing it. I think the linux rule (the whole line, not including the parts that are removed on commit, should not exceed 75 characters) is far more sensible - which is why this draft states this. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, Feb 03, 2020 at 02:54:05PM +0300, Alexander Monakov wrote: > On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > > > I've not seen any follow-up to this version. Should we go ahead and adopt > > this? > > Can we please go with 'committed' (lowercase) rather than all-caps COMMITTED? > Spelling this with all-caps seems like a recent thing on gcc-patches, before > everyone used the lowercase version, which makes more sense (no need to shout > about the thing that didn't need any discussion before applying the patch). Lower case certainly makes more sense. None of this are *rules*. We should not pretend they are. An email subject should be useful to what the receivers of that email use it for: see if it very interesting to them, see if it probably not interesting to them: that's the "smth: " at the start, and the PR number at the end, and of course the actual subject itself, so we should not put in too much fluff in the subject, there needs to be room left (in the less than fifty chars total) for an actual subject :-) (The example in the patch does not capitalise the subject line, btw. It should.) Segher
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > Upper case is what glibc has, though it appears that it's a rule that is not > strictly followed. If we change it, then it becomes another friction point > between developer groups. Personally, I'd leave it as is, then turn a blind > eye to such minor non-conformance. In that case can we simply say that both 'committed' and 'COMMITTED' are okay, if we know glibc doesn't follow that rule and anticipate we will not follow it either? Thanks. Alexander
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 03/02/2020 11:54, Alexander Monakov wrote: On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: I've not seen any follow-up to this version. Should we go ahead and adopt this? Can we please go with 'committed' (lowercase) rather than all-caps COMMITTED? Spelling this with all-caps seems like a recent thing on gcc-patches, before everyone used the lowercase version, which makes more sense (no need to shout about the thing that didn't need any discussion before applying the patch). Also, while tools like 'git format-patch' will automatically put [PATCH] in the subject, for '[COMMITTED]' it will be the human typing that out, and it makes little sense to require people to meticulously type that out in caps. Especially when the previous practice was opposite. Thanks. Alexander Upper case is what glibc has, though it appears that it's a rule that is not strictly followed. If we change it, then it becomes another friction point between developer groups. Personally, I'd leave it as is, then turn a blind eye to such minor non-conformance. Quite frankly, then bit that matters most is what follows that, since that is what gets written into the git commit message. R.
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On Mon, 3 Feb 2020, Richard Earnshaw (lists) wrote: > I've not seen any follow-up to this version. Should we go ahead and adopt > this? Can we please go with 'committed' (lowercase) rather than all-caps COMMITTED? Spelling this with all-caps seems like a recent thing on gcc-patches, before everyone used the lowercase version, which makes more sense (no need to shout about the thing that didn't need any discussion before applying the patch). Also, while tools like 'git format-patch' will automatically put [PATCH] in the subject, for '[COMMITTED]' it will be the human typing that out, and it makes little sense to require people to meticulously type that out in caps. Especially when the previous practice was opposite. Thanks. Alexander
Re: [PATCH, v3] wwwdocs: e-mail subject lines for contributions
On 22/01/2020 17:45, Richard Earnshaw (lists) wrote: [updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). I've not seen any follow-up to this version. Should we go ahead and adopt this? R.
[PATCH, v3] wwwdocs: e-mail subject lines for contributions
[updated based on v2 discussions] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). diff --git a/htdocs/contribute.html b/htdocs/contribute.html index 042ff069..558bae29 100644 --- a/htdocs/contribute.html +++ b/htdocs/contribute.html @@ -249,13 +249,143 @@ that ChangeLog entries may be included as part of the patch and diffs representing new files may be omitted, especially if large, since they can be accessed directly from the repository.) +E-mail subject lines + +Your contribution e-mail subject line will become the first line of +the commit message for your patch. + +A high-quality e-mail subject line for a contribution contains the +following elements: + + + A classifier + Component tags + An optional series identifier + A brief summary + An optional bug number + + +The entire line (excluding the classifier) should not exceed 75 +characters. + +Classifier + +The classifier identifies the type of contribution, for example a +patch, an RFC (request for comments) or a committed patch (where +approval is not necessary. The classifier should be written in upper +case and surrounded with square brackets. This is the only component +of the e-mail subject line that will not appear in the commit itself. +The classifier may optionally contain a version number (vN) and +a series marker (N/M). Examples are: + + + [PATCH] - a single patch + [PATCH v2] - the second version of a single patch + [PATCH 3/7] - the third patch in a series of seven +patches + [RFC] - a point of discussion, may contain a patch + [COMMITTED] - a patch that has already been committed. + + +Component tags + +A component tag is a short identifier that identifies the part of +the compiler being modified. This highlights to the relevant +maintainers that the patch may need their attention. Multiple +components may be listed if necessary. Each component tag should be +followed by a colon. For example, + + + libstdc++: + combine: + vax: testsuite: + + +Some large components may be subdivided into sub-components. If +the subcomponent name is not disctinct in its own right, you can use the +form component/sub-component:. + +Series identifier + +The series identifier is optional and is only relevant if a number of +patches are needed in order to effect an overall change. It should be +a short string that identifies the series (it is common to all +patches) and should be followed by a single dash surrounded by white +space. + +A Very Brief summary + +The brief summary encapsulates in a few words the intent of the +change. For example: cleanup check_field_decls. +Although, very short, the summary should be distinct so that it will +not be confused with other patches. + +Bug number + +If your patch relates a bug in the compiler for which there is an +existing PR number the bug number should be stated. Use the +short-form variant [PRn] without the bugzilla +component identifier and with no space between 'PR' and the number. +The body of the commit message should still contain the full form +(PR component/n) within the body of +the commit message so that Bugzilla will correctly notice the +commit. If your patch relates to two bugs, then write +[PRn, PRm]. For multiple +bugs, just cite the most relevant one in the summary and use an +elipsis instead of the second, or subsequent PR numbers; list all the +related PRs in the body of the commit message in the normal way. + +It is not necessary to cite bugs that are closed as duplicates of +another unless there is something specific to that report that +is not covered by the parent bug. + +Other messages + +Some large patch sets benefit from an introductory e-mail that +provides more context for the patch series and describes how the +patches have been broken up to provide for review. The convention is +that such messages should follow the same format as described above, +but the patch number should be set to zero, for example: [PATCH +0/7]. Remember that the introductory message will not be +committed with the patches themselves, so it should not contain any +important information that is not also covered in the individual +patches. If you send a summary e-mail with a series
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 22/01/2020 16:28, Marek Polacek wrote: On Wed, Jan 22, 2020 at 04:05:37PM +, Richard Sandiford wrote: "Richard Earnshaw (lists)" writes: On 21/01/2020 17:20, Jason Merrill wrote: On 1/21/20 10:40 AM, Richard Earnshaw (lists) wrote: On 21/01/2020 15:39, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) wrote: Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not too worried. I'd be happy with [PR #], but if folk want something else, please say so quickly... [PR 12345] or [PR #12345] is bad, because the bugzilla won't underline it, it needs to be either PR12345 word, or PR component/12345 . ok, lets go with [PR] then. Doesn't this use of [] have the same problem with git am? No, because only 'leading' [] blocks are removed - git mailinfo --help My summaries are often describing the bug I'm fixing, i.e. [PATCH] PR c++/91476 - anon-namespace reference temp clash between TUs. which is also the first line of my ChangeLog entry. I think you are proposing [COMMITTED] c++: Fix anon-namespace reference temp clash between TUs (PR91476) which can no longer be shared with the ChangeLog. I was trying to unify this with glibc. They specify the bug number at the end of the line. We can diverge if it's generally felt to be important, but details like this create needless friction for folk working in both communities. +1 for "component: Summary [PRn]" FWIW. PR bz-component/n works well for C++. The problem is that so many other PRs come under tree-optimization and rtl-optimization, which eat up a lot of subject line characters without narrowing things down very much. "cselib: ... [PRn]" is both shorter and more descriptive than "PR rtl-optimization/n - ", etc. Yeah, the cselib version definitely looks preferable to me. What if a patch touches more than just the C++ FE, do we want "c,c++:"? c-common: or c-family:? Though kernel uses net: sched: act_ctinfo: fix memory leak I'd read that as a patch that crosses the three separate components. locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN And this would be a patch that affectst the rwsem sub-component of locking. If a patch touches various spots in the optimizers, maybe we can just go with "tree-opt:" or "rtl:"? Not sure we want to get that prescriptive. Things are likely to change around anyway. rtl: would suggest it was one of the non-specific rtl-related issues, tree: similarly for a tree-related issue. Further, I suppose multiple PRs fixed by a single patch would look like: c++: Implement DR 666 [PR57, PR12345] Depends on the overall context. In the subject line I think it would be acceptable to reference just one, perhaps two PRs. If there are more, then something like [PRnnn, ...] would probably be an acceptable way of showing that more were referenced later in the body. Marek
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Wed, Jan 22, 2020 at 04:05:37PM +, Richard Sandiford wrote: > "Richard Earnshaw (lists)" writes: > > On 21/01/2020 17:20, Jason Merrill wrote: > >> On 1/21/20 10:40 AM, Richard Earnshaw (lists) wrote: > >>> On 21/01/2020 15:39, Jakub Jelinek wrote: > >>>> On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) > >>>> wrote: > >>>>>> Some examples would be useful I'd say, e.g. it is unclear in what > >>>>>> way you > >>>>>> want the PR number to be appended, shall it be > >>>>>> something: whatever words describe it PR12345 > >>>>>> or > >>>>>> something: whatever words describe it (PR12345) > >>>>>> or > >>>>>> something: whatever words describe it: PR12345 > >>>>>> or > >>>>>> something: whatever words describe it [PR12345] > >>>>>> or something else? > >>>>> > >>>>> Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, > >>>>> I'm not > >>>>> too worried. I'd be happy with [PR #], but if folk want > >>>>> something else, > >>>>> please say so quickly... > >>>> > >>>> [PR 12345] or [PR #12345] is bad, because the bugzilla won't > >>>> underline it, > >>>> it needs to be either PR12345 word, or PR component/12345 . > >>> > >>> ok, lets go with [PR] then. > >> > >> Doesn't this use of [] have the same problem with git am? > > > > No, because only 'leading' [] blocks are removed - git mailinfo --help > > > >> > >> My summaries are often describing the bug I'm fixing, i.e. > >> > >> [PATCH] PR c++/91476 - anon-namespace reference temp clash between TUs. > >> > >> which is also the first line of my ChangeLog entry. I think you are > >> proposing > >> > >> [COMMITTED] c++: Fix anon-namespace reference temp clash between TUs > >> (PR91476) > >> > >> which can no longer be shared with the ChangeLog. > >> > > > > I was trying to unify this with glibc. They specify the bug number at > > the end of the line. > > > > We can diverge if it's generally felt to be important, but details like > > this create needless friction for folk working in both communities. > > +1 for "component: Summary [PRn]" FWIW. > > PR bz-component/n works well for C++. The problem is that so many > other PRs come under tree-optimization and rtl-optimization, which > eat up a lot of subject line characters without narrowing things down > very much. "cselib: ... [PRn]" is both shorter and more descriptive > than "PR rtl-optimization/n - ", etc. Yeah, the cselib version definitely looks preferable to me. What if a patch touches more than just the C++ FE, do we want "c,c++:"? Though kernel uses net: sched: act_ctinfo: fix memory leak locking/rwsem: Fix kernel crash when spinning on RWSEM_OWNER_UNKNOWN If a patch touches various spots in the optimizers, maybe we can just go with "tree-opt:" or "rtl:"? Further, I suppose multiple PRs fixed by a single patch would look like: c++: Implement DR 666 [PR57, PR12345] Marek
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
"Richard Earnshaw (lists)" writes: > On 21/01/2020 17:20, Jason Merrill wrote: >> On 1/21/20 10:40 AM, Richard Earnshaw (lists) wrote: >>> On 21/01/2020 15:39, Jakub Jelinek wrote: >>>> On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) >>>> wrote: >>>>>> Some examples would be useful I'd say, e.g. it is unclear in what >>>>>> way you >>>>>> want the PR number to be appended, shall it be >>>>>> something: whatever words describe it PR12345 >>>>>> or >>>>>> something: whatever words describe it (PR12345) >>>>>> or >>>>>> something: whatever words describe it: PR12345 >>>>>> or >>>>>> something: whatever words describe it [PR12345] >>>>>> or something else? >>>>> >>>>> Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, >>>>> I'm not >>>>> too worried. I'd be happy with [PR #], but if folk want >>>>> something else, >>>>> please say so quickly... >>>> >>>> [PR 12345] or [PR #12345] is bad, because the bugzilla won't >>>> underline it, >>>> it needs to be either PR12345 word, or PR component/12345 . >>> >>> ok, lets go with [PR] then. >> >> Doesn't this use of [] have the same problem with git am? > > No, because only 'leading' [] blocks are removed - git mailinfo --help > >> >> My summaries are often describing the bug I'm fixing, i.e. >> >> [PATCH] PR c++/91476 - anon-namespace reference temp clash between TUs. >> >> which is also the first line of my ChangeLog entry. I think you are >> proposing >> >> [COMMITTED] c++: Fix anon-namespace reference temp clash between TUs >> (PR91476) >> >> which can no longer be shared with the ChangeLog. >> > > I was trying to unify this with glibc. They specify the bug number at > the end of the line. > > We can diverge if it's generally felt to be important, but details like > this create needless friction for folk working in both communities. +1 for "component: Summary [PRn]" FWIW. PR bz-component/n works well for C++. The problem is that so many other PRs come under tree-optimization and rtl-optimization, which eat up a lot of subject line characters without narrowing things down very much. "cselib: ... [PRn]" is both shorter and more descriptive than "PR rtl-optimization/n - ", etc. Same idea for "PR target/n - ...": you then need to say which target you mean in the main summary, whereas "aarch64: [PRn]" makes it easier to keep the main summary short. Maybe that's just a problem with the bz classification though... Richard
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Wed, Jan 22, 2020 at 10:00:00AM +, Richard Earnshaw (lists) wrote: > On 21/01/2020 19:26, Segher Boessenkool wrote: > >On Tue, Jan 21, 2020 at 02:52:00PM +, Richard Earnshaw (lists) wrote: > >>+ A brief summary > > > >You could stress that this is the one thing that really matters. And > >it's not a summary, it's much too brief for that (at most ~50 chars), > >but yup it should be something that allows *a human* to identify what > >this is. > > > > Well, it should all matter, or why are we requiring it? Yes. > I'm happy to insert 'very' in front of 'brief' and to say elsewhere that > the entire subject (less the leading [...] part, should rarely, if ever, > go beyond 80 characters. The usual recommendation is 50 chars. Which is just about right with most MUAs. > >Everything else is just convention. > > > >>+A component tag is a short identifier that identifies the part of > >>+the compiler being modified. This highlights to the relevant > >>+maintainers that the patch may need their attention. Multiple > >>+components may be listed if necessary. Each component tag should be > >>+followed by a colon. > > > >Often people use aaa/bbb: if drilling down a bit that way helps keep the > >subject short (which is the *point* of all this: keep things better > >consumable for humans). > > The aaa: bbb: is really for when aaa and bbb are independent parts of > the compiler and potentially needs multiple reviewers. aaa/bbb is when > bbb is strictly a sub-component of aaa (for example, arm/SVE: would be > an SVE related issue in the arm backend). I'm certainly not against > having that. Excellent. > >>+The brief summary encapsulates in a few words the intent of the > >>+change. For example: cleanup check_field_decls. > > > >It should start with a capital though. "Clean up blablal". (So no > >dot to end the sentence, this isn't a sentence). A capital helps > >the reader to quickly identify what is what, separate fluff from the > >core parts. > > There is a balance here to be made. I'm mindful of Gerald's concern > that this is perhaps overly restrictive for new users already. I > certainly think we should have a good house style, but getting too > prescriptive just gets in the way of attracting good contributions. This is just basic email etiquette :-) But, it's very annoying when you look at badly formatted subjects, in "git log --oneline" for example, it is very obvious there (and long subjects are problematic there as well btw). Wrt balance: yes, that is one reason why I do not think we should require all the markup stuff. > >>+Some large patch sets benefit from an introductory e-mail that > >>+provides more context for the patch series and describes how the > >>+patches have been broken up to provide for review. > > > >All non-trivial series, yeah. > > > >Maybe we should mention how v2 etc. of patch series should show what is > >changed? If there is a good standard practice for that at all :-) > > Dunno. I think that belongs primarily in the v2 0/n mail. It's not, > after all, something that needs to be kept once the patches are applied. Sure. Ah, we could recommend that then, to make it clear in the vM 0/N of some series which patches changed how. My point is that it is a waste of time to everyone if a reviewer has to drag through everything every time; this is double true with git, it is easy to send updated versions of patch sets often. Segher
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 21/01/2020 19:26, Segher Boessenkool wrote: Hi! Thanks for doing this. On Tue, Jan 21, 2020 at 02:52:00PM +, Richard Earnshaw (lists) wrote: This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. +A high-quality e-mail subject line for a contribution contains the +following elements: + A brief summary You could stress that this is the one thing that really matters. And it's not a summary, it's much too brief for that (at most ~50 chars), but yup it should be something that allows *a human* to identify what this is. Well, it should all matter, or why are we requiring it? I'm happy to insert 'very' in front of 'brief' and to say elsewhere that the entire subject (less the leading [...] part, should rarely, if ever, go beyond 80 characters. Everything else is just convention. +A component tag is a short identifier that identifies the part of +the compiler being modified. This highlights to the relevant +maintainers that the patch may need their attention. Multiple +components may be listed if necessary. Each component tag should be +followed by a colon. Often people use aaa/bbb: if drilling down a bit that way helps keep the subject short (which is the *point* of all this: keep things better consumable for humans). The aaa: bbb: is really for when aaa and bbb are independent parts of the compiler and potentially needs multiple reviewers. aaa/bbb is when bbb is strictly a sub-component of aaa (for example, arm/SVE: would be an SVE related issue in the arm backend). I'm certainly not against having that. +The brief summary encapsulates in a few words the intent of the +change. For example: cleanup check_field_decls. It should start with a capital though. "Clean up blablal". (So no dot to end the sentence, this isn't a sentence). A capital helps the reader to quickly identify what is what, separate fluff from the core parts. There is a balance here to be made. I'm mindful of Gerald's concern that this is perhaps overly restrictive for new users already. I certainly think we should have a good house style, but getting too prescriptive just gets in the way of attracting good contributions. +Some large patch sets benefit from an introductory e-mail that +provides more context for the patch series and describes how the +patches have been broken up to provide for review. All non-trivial series, yeah. Maybe we should mention how v2 etc. of patch series should show what is changed? If there is a good standard practice for that at all :-) Dunno. I think that belongs primarily in the v2 0/n mail. It's not, after all, something that needs to be kept once the patches are applied. R. Segher
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Wed, Jan 22, 2020 at 09:35:31AM +, Richard Earnshaw (lists) wrote: > On 22/01/2020 09:07, Jakub Jelinek wrote: > > On Tue, Jan 21, 2020 at 06:50:13PM +, Richard Earnshaw (lists) wrote: > > > > Doesn't this use of [] have the same problem with git am? > > > > > > No, because only 'leading' [] blocks are removed - git mailinfo --help > > > > I've used > > openmp: Teach omp_code_to_statement about rest of OpenMP statements > > [PR93329] > > as the first line of a commit log and that [PR93329] part is completely gone > > from it. > > How did you apply the patch? Just git commit -a and editing the log message. But I got the [PR.] not eaten in another commit, so ignore the above for now (although I'm very much convinced it was in there when I wrote the commit log). Jakub
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 22/01/2020 09:07, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 06:50:13PM +, Richard Earnshaw (lists) wrote: Doesn't this use of [] have the same problem with git am? No, because only 'leading' [] blocks are removed - git mailinfo --help I've used openmp: Teach omp_code_to_statement about rest of OpenMP statements [PR93329] as the first line of a commit log and that [PR93329] part is completely gone from it. Jakub How did you apply the patch? R.
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Tue, Jan 21, 2020 at 06:50:13PM +, Richard Earnshaw (lists) wrote: > > Doesn't this use of [] have the same problem with git am? > > No, because only 'leading' [] blocks are removed - git mailinfo --help I've used openmp: Teach omp_code_to_statement about rest of OpenMP statements [PR93329] as the first line of a commit log and that [PR93329] part is completely gone from it. Jakub
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
Hi! Thanks for doing this. On Tue, Jan 21, 2020 at 02:52:00PM +, Richard Earnshaw (lists) wrote: > This patch proposes some new (additional) rules for email subject lines > when contributing to GCC. The goal is to make sure that, as far as > possible, the subject for a patch will form a good summary when the > message is committed to the repository if applied with 'git am'. > +A high-quality e-mail subject line for a contribution contains the > +following elements: > + A brief summary You could stress that this is the one thing that really matters. And it's not a summary, it's much too brief for that (at most ~50 chars), but yup it should be something that allows *a human* to identify what this is. Everything else is just convention. > +A component tag is a short identifier that identifies the part of > +the compiler being modified. This highlights to the relevant > +maintainers that the patch may need their attention. Multiple > +components may be listed if necessary. Each component tag should be > +followed by a colon. Often people use aaa/bbb: if drilling down a bit that way helps keep the subject short (which is the *point* of all this: keep things better consumable for humans). > +The brief summary encapsulates in a few words the intent of the > +change. For example: cleanup check_field_decls. It should start with a capital though. "Clean up blablal". (So no dot to end the sentence, this isn't a sentence). A capital helps the reader to quickly identify what is what, separate fluff from the core parts. > +Some large patch sets benefit from an introductory e-mail that > +provides more context for the patch series and describes how the > +patches have been broken up to provide for review. All non-trivial series, yeah. Maybe we should mention how v2 etc. of patch series should show what is changed? If there is a good standard practice for that at all :-) Segher
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 21/01/2020 17:20, Jason Merrill wrote: On 1/21/20 10:40 AM, Richard Earnshaw (lists) wrote: On 21/01/2020 15:39, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) wrote: Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not too worried. I'd be happy with [PR #], but if folk want something else, please say so quickly... [PR 12345] or [PR #12345] is bad, because the bugzilla won't underline it, it needs to be either PR12345 word, or PR component/12345 . ok, lets go with [PR] then. Doesn't this use of [] have the same problem with git am? No, because only 'leading' [] blocks are removed - git mailinfo --help My summaries are often describing the bug I'm fixing, i.e. [PATCH] PR c++/91476 - anon-namespace reference temp clash between TUs. which is also the first line of my ChangeLog entry. I think you are proposing [COMMITTED] c++: Fix anon-namespace reference temp clash between TUs (PR91476) which can no longer be shared with the ChangeLog. I was trying to unify this with glibc. They specify the bug number at the end of the line. We can diverge if it's generally felt to be important, but details like this create needless friction for folk working in both communities. R.
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 1/21/20 10:40 AM, Richard Earnshaw (lists) wrote: On 21/01/2020 15:39, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) wrote: Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not too worried. I'd be happy with [PR #], but if folk want something else, please say so quickly... [PR 12345] or [PR #12345] is bad, because the bugzilla won't underline it, it needs to be either PR12345 word, or PR component/12345 . ok, lets go with [PR] then. Doesn't this use of [] have the same problem with git am? My summaries are often describing the bug I'm fixing, i.e. [PATCH] PR c++/91476 - anon-namespace reference temp clash between TUs. which is also the first line of my ChangeLog entry. I think you are proposing [COMMITTED] c++: Fix anon-namespace reference temp clash between TUs (PR91476) which can no longer be shared with the ChangeLog. Jason
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 21/01/2020 15:39, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) wrote: Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not too worried. I'd be happy with [PR #], but if folk want something else, please say so quickly... [PR 12345] or [PR #12345] is bad, because the bugzilla won't underline it, it needs to be either PR12345 word, or PR component/12345 . Jakub ok, lets go with [PR] then. R.
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Tue, Jan 21, 2020 at 03:33:22PM +, Richard Earnshaw (lists) wrote: > > Some examples would be useful I'd say, e.g. it is unclear in what way you > > want the PR number to be appended, shall it be > > something: whatever words describe it PR12345 > > or > > something: whatever words describe it (PR12345) > > or > > something: whatever words describe it: PR12345 > > or > > something: whatever words describe it [PR12345] > > or something else? > > Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not > too worried. I'd be happy with [PR #], but if folk want something else, > please say so quickly... [PR 12345] or [PR #12345] is bad, because the bugzilla won't underline it, it needs to be either PR12345 word, or PR component/12345 . Jakub
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On 21/01/2020 15:04, Jakub Jelinek wrote: On Tue, Jan 21, 2020 at 02:52:00PM +, Richard Earnshaw (lists) wrote: [updated, following some comments from Gerald, main differences are slight tweaks to the html markup and changing "email" to "e-mail"] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Glibc use "[BZ #]" - obviously BZ becomes PR, but after that, I'm not too worried. I'd be happy with [PR #], but if folk want something else, please say so quickly... I'll prepare an edit for that... Also, it would be nice to stress that the PR long form should be in the ChangeLog and somewhere on the later lines of the commit message, I don't think we pick up the shorter form from the first line when it short form (I could be wrong, but e.g. https://gcc.gnu.org/g:865257c447cc50f5819e9b53da83145f3c36c305 commit didn't make it into bugzilla). Yes, good point. Thanks. R.
Re: [PATCH, v2] wwwdocs: e-mail subject lines for contributions
On Tue, Jan 21, 2020 at 02:52:00PM +, Richard Earnshaw (lists) wrote: > [updated, following some comments from Gerald, main differences are > slight tweaks to the html markup and changing "email" to "e-mail"] > > This patch proposes some new (additional) rules for email subject lines > when contributing to GCC. The goal is to make sure that, as far as > possible, the subject for a patch will form a good summary when the > message is committed to the repository if applied with 'git am'. Where > possible, I've tried to align these rules with those already in > use for glibc, so that the differences are minimal and only where > necessary. > > Some things that differ from existing practice (at least by some people) > are: > > - Use ':' rather than '[]' > - This is more git friendly and works with 'git am'. > - Put bug numbers at the end of the line rather than the beginning. > - The bug number is useful, but not as useful as the brief summary. > Also, use the shortened form, as the topic part is more usefully > conveyed in the proper topic field (see above). Some examples would be useful I'd say, e.g. it is unclear in what way you want the PR number to be appended, shall it be something: whatever words describe it PR12345 or something: whatever words describe it (PR12345) or something: whatever words describe it: PR12345 or something: whatever words describe it [PR12345] or something else? Also, it would be nice to stress that the PR long form should be in the ChangeLog and somewhere on the later lines of the commit message, I don't think we pick up the shorter form from the first line when it short form (I could be wrong, but e.g. https://gcc.gnu.org/g:865257c447cc50f5819e9b53da83145f3c36c305 commit didn't make it into bugzilla). Jakub
[PATCH, v2] wwwdocs: e-mail subject lines for contributions
[updated, following some comments from Gerald, main differences are slight tweaks to the html markup and changing "email" to "e-mail"] This patch proposes some new (additional) rules for email subject lines when contributing to GCC. The goal is to make sure that, as far as possible, the subject for a patch will form a good summary when the message is committed to the repository if applied with 'git am'. Where possible, I've tried to align these rules with those already in use for glibc, so that the differences are minimal and only where necessary. Some things that differ from existing practice (at least by some people) are: - Use ':' rather than '[]' - This is more git friendly and works with 'git am'. - Put bug numbers at the end of the line rather than the beginning. - The bug number is useful, but not as useful as the brief summary. Also, use the shortened form, as the topic part is more usefully conveyed in the proper topic field (see above). diff --git a/htdocs/contribute.html b/htdocs/contribute.html index 042ff069..861f7e5e 100644 --- a/htdocs/contribute.html +++ b/htdocs/contribute.html @@ -249,13 +249,98 @@ that ChangeLog entries may be included as part of the patch and diffs representing new files may be omitted, especially if large, since they can be accessed directly from the repository.) +E-mail subject lines + +Your contribution e-mail subject line will become the first line of +the commit message for your patch. + +A high-quality e-mail subject line for a contribution contains the +following elements: + + + A classifier + Component tags + An optional series identifier + A brief summary + An optional bug number + + +Classifier + +The classifier identifies the type of contribution, for example a +patch, an RFC (request for comments) or a committed patch (where +approval is not necessary. The classifier should be written in upper +case and surrounded with square brackets. This is the only component +of the e-mail subject line that will not appear in the commit itself. +The classifier may optionally contain a version number (vN) and +a series marker (N/M). Examples are: + + + [PATCH] - a single patch + [PATCH v2] - the second version of a single patch + [PATCH 3/7] - the third patch in a series of seven +patches + [RFC] - a point of discussion, may contain a patch + [COMMITTED] - a patch that has already been committed. + + +Component tags + +A component tag is a short identifier that identifies the part of +the compiler being modified. This highlights to the relevant +maintainers that the patch may need their attention. Multiple +components may be listed if necessary. Each component tag should be +followed by a colon. For example, + + + libstdc++: + combine: + vax: testsuite: + + +Series identifier + +The series identifier is optional and is only relevant if a number of +patches are needed in order to effect an overall change. It should be +a short string that identifies the series (it is common to all +patches) and should be followed by a single dash surrounded by white +space. + +Brief summary + +The brief summary encapsulates in a few words the intent of the +change. For example: cleanup check_field_decls. + +Bug number + +If your patch fixes a bug in the compiler for which there is an +existing PR number the bug number should be stated. Use the +short-form variant (PRn) without the bugzilla component +identifier. + +Other messages + +Some large patch sets benefit from an introductory e-mail that +provides more context for the patch series and describes how the +patches have been broken up to provide for review. The convention is +that such messages should follow the same format as described above, +but the patch number should be set to zero, for example: [PATCH +0/7]. Remember that the introductory message will not be +committed with the patches themselves, so it should not contain any +important information that is not also covered in the individual +patches. If you send a summary e-mail with a series it is a good idea +to send the patches as follow-ups (essentially replies) to your +initial message so that mail software can group the messages +together. + Pinging patches, Getting patches applied If you do not receive a response to a patch that you have submitted within two weeks or so, it may be a good idea to chase it by sending a -follow-up email to the same list(s). Patches can occasionally fall through -the cracks. Please be sure to include a brief summary of the patch and the -URL of the entry in the mailing list archive of the original submission. +follow-up e-mail to the same list(s). Patches can occasionally fall +through the cracks. Please be sure to include a brief summary of the +patch and the URL of the entry in the mailing list archive of the +original submission. If you do not have write access and a patch of yours has been approved, but not committed, please advise the approver of
[PATCH] V6, #16 of 17: Wrong subject, should have been update @pcrel
Note, patch #16 had the wrong subject line. It should have been that modifies @pcrel to use an explicit (0),1. Sorry about that. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
Re: Subject: [PATCH] [PR 89330] Remove non-useful speculations from new_edges
Hi Martin,
> the following patch prevents the call speculation machinery from
> deallocating call graph edges from under the indirect inlining machinery
> and it also fixes a potential issue in speculation which could in some
> cases undo an earlier inlining decision, something that the inliner is
> not built to expect.
>
> That latter change requires disabling speculation on a testcase through
> adding -fno-profile-values, otherwise a devirtualziation happens before
> it is dump-scanned for.
>
> Bootstrapped and tested on an x86_64-linux, OK for trunk?
[...]
> 2019-07-25 Martin Jambor
>
> PR ipa/89330
> * ipa-inline-transform.c (check_speculations_1): New function.
> (push_all_edges_in_set_to_vec): Likewise.
> (check_speculations): Use check_speculations_1, new parameter
> new_edges.
> (inline_call): Pass new_edges to check_speculations.
> * ipa-inline.c (add_new_edges_to_heap): Assert edge_callee is not
> NULL.
> (speculation_useful_p): Early return true if edge is inlined, remove
> later checks for inline_failed.
>
> testsuite/
> * g++.dg/lto/pr89330_[01].C: New test.
the new test FAILs on Solaris:
+FAIL: g++.dg/lto/pr89330 cp_lto_pr89330_0.o-cp_lto_pr89330_1.o link, -O3 -g
-flto -shared -Wno-odr
Text relocation remains referenced
against symbol offset in file
Inkscape::XML::Node::root() 0x9ccp_lto_pr89330_1.o
virtual thunk to Inkscape::XML::SimpleNode::name() const 0x6c
cp_lto_pr89330_1.o
Inkscape::XML::Node::parent() 0xa4cp_lto_pr89330_1.o
Inkscape::XML::Node::document() const 0x98 cp_lto_pr89330_1.o
Inkscape::XML::Node::next() 0xaccp_lto_pr89330_1.o
[...]
Inkscape::XML::Node::code() 0x70cp_lto_pr89330_1.o
Inkscape::XML::SimpleNode::name() const 0xc cp_lto_pr89330_1.o
Inkscape::XML::Node::document() 0x94cp_lto_pr89330_1.o
ld: fatal: relocations remain against allocatable but non-writable sections
collect2: error: ld returned 1 exit status
This happens because ld defaults to -z text here and can easily be
avoided by compiling the code as PIC.
This is what the following patch does. Tested on i386-pc-solaris2.11,
sparc-sun-solaris2.11, and x86_64-pc-linux-gnu, installed on mainline.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
2019-07-28 Rainer Orth
* g++.dg/lto/pr89330_0.C (dg-lto-options): Add -fPIC.
Require fpic support.
# HG changeset patch
# Parent d121877e3d1d53929c62025b84c0a4a357901a96
Fix g++.dg/lto/pr89330 on Solaris
diff --git a/gcc/testsuite/g++.dg/lto/pr89330_0.C b/gcc/testsuite/g++.dg/lto/pr89330_0.C
--- a/gcc/testsuite/g++.dg/lto/pr89330_0.C
+++ b/gcc/testsuite/g++.dg/lto/pr89330_0.C
@@ -1,5 +1,6 @@
// { dg-lto-do link }
-// { dg-lto-options { { -O3 -g -flto -shared -Wno-odr } } }
+// { dg-lto-options { { -O3 -g -flto -shared -fPIC -Wno-odr } } }
+// { dg-require-effective-target fpic }
namespace Inkscape {
class Anchored {};
Re: Subject: [PATCH] [PR 89330] Remove non-useful speculations from new_edges
> Hi,
>
> the following patch prevents the call speculation machinery from
> deallocating call graph edges from under the indirect inlining machinery
> and it also fixes a potential issue in speculation which could in some
> cases undo an earlier inlining decision, something that the inliner is
> not built to expect.
>
> That latter change requires disabling speculation on a testcase through
> adding -fno-profile-values, otherwise a devirtualziation happens before
> it is dump-scanned for.
>
> Bootstrapped and tested on an x86_64-linux, OK for trunk?
OK,
thanks!
Honza
>
> Thanks,
>
> Martin
>
>
> 2019-07-25 Martin Jambor
>
> PR ipa/89330
> * ipa-inline-transform.c (check_speculations_1): New function.
> (push_all_edges_in_set_to_vec): Likewise.
> (check_speculations): Use check_speculations_1, new parameter
> new_edges.
> (inline_call): Pass new_edges to check_speculations.
> * ipa-inline.c (add_new_edges_to_heap): Assert edge_callee is not
> NULL.
> (speculation_useful_p): Early return true if edge is inlined, remove
> later checks for inline_failed.
>
> testsuite/
> * g++.dg/lto/pr89330_[01].C: New test.
> * g++.dg/tree-prof/devirt.C: Added -fno-profile-values to dg-options.
> ---
> gcc/ipa-inline-transform.c | 42 +++--
> gcc/ipa-inline.c| 12 --
> gcc/testsuite/g++.dg/lto/pr89330_0.C| 50 +
> gcc/testsuite/g++.dg/lto/pr89330_1.C| 36 ++
> gcc/testsuite/g++.dg/tree-prof/devirt.C | 2 +-
> 5 files changed, 133 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_0.C
> create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_1.C
>
> diff --git a/gcc/ipa-inline-transform.c b/gcc/ipa-inline-transform.c
> index 4a3a193bc9c..897c563f19a 100644
> --- a/gcc/ipa-inline-transform.c
> +++ b/gcc/ipa-inline-transform.c
> @@ -237,10 +237,13 @@ clone_inlined_nodes (struct cgraph_edge *e, bool
> duplicate,
> }
> }
>
> -/* Check all speculations in N and resolve them if they seems useless. */
> +/* Check all speculations in N and if any seem useless, resolve them. When a
> + first edge is resolved, pop all edges from NEW_EDGES and insert them to
> + EDGE_SET. Then remove each resolved edge from EDGE_SET, if it is there.
> */
>
> static bool
> -check_speculations (cgraph_node *n)
> +check_speculations_1 (cgraph_node *n, vec *new_edges,
> + hash_set *edge_set)
> {
>bool speculation_removed = false;
>cgraph_edge *next;
> @@ -250,15 +253,46 @@ check_speculations (cgraph_node *n)
>next = e->next_callee;
>if (e->speculative && !speculation_useful_p (e, true))
> {
> + while (new_edges && !new_edges->is_empty ())
> + edge_set->add (new_edges->pop ());
> + edge_set->remove (e);
> +
> e->resolve_speculation (NULL);
> speculation_removed = true;
> }
>else if (!e->inline_failed)
> - speculation_removed |= check_speculations (e->callee);
> + speculation_removed |= check_speculations_1 (e->callee, new_edges,
> + edge_set);
> }
>return speculation_removed;
> }
>
> +/* Push E to NEW_EDGES. Called from hash_set traverse method, which
> + unfortunately means this function has to have external linkage, otherwise
> + the code will not compile with gcc 4.8. */
> +
> +bool
> +push_all_edges_in_set_to_vec (cgraph_edge * const ,
> + vec *new_edges)
> +{
> + new_edges->safe_push (e);
> + return true;
> +}
> +
> +/* Check all speculations in N and if any seem useless, resolve them and
> remove
> + them from NEW_EDGES. */
> +
> +static bool
> +check_speculations (cgraph_node *n, vec *new_edges)
> +{
> + hash_set edge_set;
> + bool res = check_speculations_1 (n, new_edges, _set);
> + if (!edge_set.is_empty ())
> +edge_set.traverse *,
> +push_all_edges_in_set_to_vec> (new_edges);
> + return res;
> +}
> +
> /* Mark all call graph edges coming out of NODE and all nodes that have been
> inlined to it as in_polymorphic_cdtor. */
>
> @@ -450,7 +484,7 @@ inline_call (struct cgraph_edge *e, bool update_original,
> mark_all_inlined_calls_cdtor (e->callee);
>if (opt_for_fn (e->caller->decl, optimize))
> new_edges_found = ipa_propagate_indirect_call_infos (curr, new_edges);
> - check_speculations (e->callee);
> + check_speculations (e->callee, new_edges);
>if (update_overall_summary)
> ipa_update_overall_fn_summary (to);
>else
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index 5862d8d..0f1699a182f 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -1626,6 +1626,7 @@ add_new_edges_to_heap (edge_heap_t *heap,
> vec new_edges)
>struct cgraph_edge *edge = new_edges.pop ();
>
>gcc_assert (!edge->aux);
> +
Subject: [PATCH] [PR 89330] Remove non-useful speculations from new_edges
Hi,
the following patch prevents the call speculation machinery from
deallocating call graph edges from under the indirect inlining machinery
and it also fixes a potential issue in speculation which could in some
cases undo an earlier inlining decision, something that the inliner is
not built to expect.
That latter change requires disabling speculation on a testcase through
adding -fno-profile-values, otherwise a devirtualziation happens before
it is dump-scanned for.
Bootstrapped and tested on an x86_64-linux, OK for trunk?
Thanks,
Martin
2019-07-25 Martin Jambor
PR ipa/89330
* ipa-inline-transform.c (check_speculations_1): New function.
(push_all_edges_in_set_to_vec): Likewise.
(check_speculations): Use check_speculations_1, new parameter
new_edges.
(inline_call): Pass new_edges to check_speculations.
* ipa-inline.c (add_new_edges_to_heap): Assert edge_callee is not
NULL.
(speculation_useful_p): Early return true if edge is inlined, remove
later checks for inline_failed.
testsuite/
* g++.dg/lto/pr89330_[01].C: New test.
* g++.dg/tree-prof/devirt.C: Added -fno-profile-values to dg-options.
---
gcc/ipa-inline-transform.c | 42 +++--
gcc/ipa-inline.c| 12 --
gcc/testsuite/g++.dg/lto/pr89330_0.C| 50 +
gcc/testsuite/g++.dg/lto/pr89330_1.C| 36 ++
gcc/testsuite/g++.dg/tree-prof/devirt.C | 2 +-
5 files changed, 133 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_0.C
create mode 100644 gcc/testsuite/g++.dg/lto/pr89330_1.C
diff --git a/gcc/ipa-inline-transform.c b/gcc/ipa-inline-transform.c
index 4a3a193bc9c..897c563f19a 100644
--- a/gcc/ipa-inline-transform.c
+++ b/gcc/ipa-inline-transform.c
@@ -237,10 +237,13 @@ clone_inlined_nodes (struct cgraph_edge *e, bool
duplicate,
}
}
-/* Check all speculations in N and resolve them if they seems useless. */
+/* Check all speculations in N and if any seem useless, resolve them. When a
+ first edge is resolved, pop all edges from NEW_EDGES and insert them to
+ EDGE_SET. Then remove each resolved edge from EDGE_SET, if it is there. */
static bool
-check_speculations (cgraph_node *n)
+check_speculations_1 (cgraph_node *n, vec *new_edges,
+ hash_set *edge_set)
{
bool speculation_removed = false;
cgraph_edge *next;
@@ -250,15 +253,46 @@ check_speculations (cgraph_node *n)
next = e->next_callee;
if (e->speculative && !speculation_useful_p (e, true))
{
+ while (new_edges && !new_edges->is_empty ())
+ edge_set->add (new_edges->pop ());
+ edge_set->remove (e);
+
e->resolve_speculation (NULL);
speculation_removed = true;
}
else if (!e->inline_failed)
- speculation_removed |= check_speculations (e->callee);
+ speculation_removed |= check_speculations_1 (e->callee, new_edges,
+edge_set);
}
return speculation_removed;
}
+/* Push E to NEW_EDGES. Called from hash_set traverse method, which
+ unfortunately means this function has to have external linkage, otherwise
+ the code will not compile with gcc 4.8. */
+
+bool
+push_all_edges_in_set_to_vec (cgraph_edge * const ,
+ vec *new_edges)
+{
+ new_edges->safe_push (e);
+ return true;
+}
+
+/* Check all speculations in N and if any seem useless, resolve them and remove
+ them from NEW_EDGES. */
+
+static bool
+check_speculations (cgraph_node *n, vec *new_edges)
+{
+ hash_set edge_set;
+ bool res = check_speculations_1 (n, new_edges, _set);
+ if (!edge_set.is_empty ())
+edge_set.traverse *,
+ push_all_edges_in_set_to_vec> (new_edges);
+ return res;
+}
+
/* Mark all call graph edges coming out of NODE and all nodes that have been
inlined to it as in_polymorphic_cdtor. */
@@ -450,7 +484,7 @@ inline_call (struct cgraph_edge *e, bool update_original,
mark_all_inlined_calls_cdtor (e->callee);
if (opt_for_fn (e->caller->decl, optimize))
new_edges_found = ipa_propagate_indirect_call_infos (curr, new_edges);
- check_speculations (e->callee);
+ check_speculations (e->callee, new_edges);
if (update_overall_summary)
ipa_update_overall_fn_summary (to);
else
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index 5862d8d..0f1699a182f 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -1626,6 +1626,7 @@ add_new_edges_to_heap (edge_heap_t *heap, vec new_edges)
struct cgraph_edge *edge = new_edges.pop ();
gcc_assert (!edge->aux);
+ gcc_assert (edge->callee);
if (edge->inline_failed
&& can_inline_edge_p (edge, true)
&& want_inline_small_function_p (edge, true)
@@ -1653,6 +1654,10 @@ heap_edge_removal_hook (struct cgraph_edge *e, void
*data)
[PING][PATCH] doc: mention that -ftls-model is subject to optimization
Ping. I've tried to phrase the additional doc text as concisely as possible.
Would a runtime warning be more appropriate?
Thanks.
On Wed, 24 Jul 2013, Alexander Monakov wrote:
Hello,
As discussed here, the current behavior of -ftls-model is intended:
http://gcc.gnu.org/ml/gcc/2013-07/msg00248.html
(in short: unlike the attribute, -ftls-model sets a most general model to be
used, but the compiler can use a less general model than specified, making it
impossible to set global-dynamic model on the command line for non-PIC code).
I'd like to add a clarification to the docs. OK for trunk?
2013-07-24 Alexander Monakov amona...@ispras.ru
* doc/invoke.texi: Mention that -ftls-model does not force the final
model.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d99d217..4d1fbee 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -20802,6 +20802,9 @@ Not all targets provide complete support for this
switch.
Alter the thread-local storage model to be used (@pxref{Thread-Local}).
The @var{model} argument should be one of @code{global-dynamic},
@code{local-dynamic}, @code{initial-exec} or @code{local-exec}.
+Note that the choice is subject to optimization: the compiler may use
+a more efficient model for symbols not visible outside of the translation
+unit, or if @option{-fpic} is not given on the command line.
The default without @option{-fpic} is @code{initial-exec}; with
@option{-fpic} the default is @code{global-dynamic}.
Re: [PATCH] doc: mention that -ftls-model is subject to optimization
On Wed, Jul 24, 2013 at 9:53 AM, Alexander Monakov amona...@ispras.ru wrote: 2013-07-24 Alexander Monakov amona...@ispras.ru * doc/invoke.texi: Mention that -ftls-model does not force the final model. This is OK. Thanks. Ian
[PATCH] doc: mention that -ftls-model is subject to optimization
Hello,
As discussed here, the current behavior of -ftls-model is intended:
http://gcc.gnu.org/ml/gcc/2013-07/msg00248.html
(in short: unlike the attribute, -ftls-model sets a most general model to be
used, but the compiler can use a less general model than specified, making it
impossible to set global-dynamic model on the command line for non-PIC code).
I'd like to add a clarification to the docs. OK for trunk?
2013-07-24 Alexander Monakov amona...@ispras.ru
* doc/invoke.texi: Mention that -ftls-model does not force the final
model.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d99d217..4d1fbee 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -20802,6 +20802,9 @@ Not all targets provide complete support for this
switch.
Alter the thread-local storage model to be used (@pxref{Thread-Local}).
The @var{model} argument should be one of @code{global-dynamic},
@code{local-dynamic}, @code{initial-exec} or @code{local-exec}.
+Note that the choice is subject to optimization: the compiler may use
+a more efficient model for symbols not visible outside of the translation
+unit, or if @option{-fpic} is not given on the command line.
The default without @option{-fpic} is @code{initial-exec}; with
@option{-fpic} the default is @code{global-dynamic}.
Fwd: Subject: Re: [Patch, fortran] PR46897 - [OOP] type-bound defined ASSIGNMENT(=) not used for derived type component in intrinsic assign
Dear All, It is only now that I see that my mail to Mikael and the release managers, to say that I would commit, bounced because of excess MIME content. I apologise for that. I can only say in mitigation that fortran is not release critical and regressions are unlikely because of the conditions that the patch hides behind. Thanks for the reviews Mikael! Committed as revision 194016. Paul -- The knack of flying is learning how to throw yourself at the ground and miss. --Hitchhikers Guide to the Galaxy
